Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Description • 7 years ago

While working on bug 1335778 I noticed that using `logger.debug()` does not always print the output the console or the log file. It sometimes work but then it doesn't.

	Andreas Tolfsen ❲:ato❳
	Comment 1 • 7 years ago

I confirmed from running test_accessibility.py (I was working on something related to it, and it specifically isn’t relevant here) that dump() works in isElementDisplayed in listener.js, but not logger.debug().

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 2 • 7 years ago

So this is actually only happening when the remoteness flag is set. In such cases any call to methods like logger.debug() will result that the output ends in the nirvana. 

Mike, do you know of an issue with log.jsm and remoteness tabs? If not, is there someone else I could ask?

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 3 • 7 years ago

So this is somehow related to the DumpAppender Marionette is using. In marionette.js we setup the logger and append one instance of DumpAppender. This works fine for modules running in the chrome process. But when the same logger is used from the framescript (listener.js) the appender doesn't seem to work in case of a remoteness tab. 

When I add a new DumpAppender in listener.js I can see the output even with remoteness turned on. So maybe we need another appender, or better create an own logger for the framescript.

	Andreas Tolfsen ❲:ato❳
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8862356 [details]
Bug 1357407 - Enable logger output for framescript when remoteness is enabled.

https://reviewboard.mozilla.org/r/134286/#review137280

::: commit-message-0f5ba:3
(Diff revision 2)
> +With remoteness enabled content framescripts don't seem to inherit the
> +appenders for loggers, which have been set by the main script. To get

This sounds like a bug.  Should we file a bug on Log.jsm?

::: testing/marionette/listener.js:95
(Diff revision 2)
>  var logger = Log.repository.getLogger("Marionette");
> +// Append only once to avoid duplicated output after listener.js gets reloaded
> +if (logger.ownAppenders.length == 0) {
> +  logger.addAppender(new Log.DumpAppender());
> +}

Do we need to do this once in every component listener.js imports, or just here?

(Feel free to drop if we don’t.)

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 7 • 7 years ago
mozreview-review-reply

Comment on attachment 8862356 [details]
Bug 1357407 - Enable logger output for framescript when remoteness is enabled.

https://reviewboard.mozilla.org/r/134286/#review137280

> This sounds like a bug.  Should we file a bug on Log.jsm?

Maybe there is something else which we do wrong? But I don't see what it should be.

Personally I would like to get feedback first, before filing a bug. Let me put a ni? for someone here on that bug.

> Do we need to do this once in every component listener.js imports, or just here?
> 
> (Feel free to drop if we don’t.)

Given that all are using the same logger, it's enough when the listener.js adds it.

	Pulsebot
	Comment 8 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e1f7afd29fcd
Enable logger output for framescript when remoteness is enabled. r=ato

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 9 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #7)
> https://reviewboard.mozilla.org/r/134286/#review137280
> 
> > This sounds like a bug.  Should we file a bug on Log.jsm?
> 
> Maybe there is something else which we do wrong? But I don't see what it
> should be.
> 
> Personally I would like to get feedback first, before filing a bug. Let me
> put a ni? for someone here on that bug.

Mike, we might still need your input, or at least you could refer to someone who knows the logger code? Thanks.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 10 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e1f7afd29fcd

	Mike Conley (:mconley) (:⚙️)
	Comment 11 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #9)
> 
> Mike, we might still need your input, or at least you could refer to someone
> who knows the logger code? Thanks.

The only thing I can think of is that sandboxing might affect logging here.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 12 • 7 years ago

Small test-only patch which improves logging for the Marionette tests. Lets get it uplifted to mozilla-beta. Thanks

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 13 • 7 years ago

(In reply to Mike Conley (:mconley) from comment #11)
> The only thing I can think of is that sandboxing might affect logging here.

Thank you Mike. It might indeed be a reason. But for now I don't think it warrants more investigation.

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/4bbb2460a2f5