918408 - marionette.log file created in users' profile folder

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Justin Dolske [:Dolske]]

Running a Firefox with a new profile creates a marionette.log file there. Deleting the log file and running Firefox again recreates it.

Why is marionette being initialized and creating this file? These are end-user systems, not test boxes. I'd expect this test framework to be checking a pref or environment variable, and only enabling itself if explicitly needed.

Comment 1

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: remove log lines when marionette is not going to be running

Thanks for noticing this, we shouldn't create that file if the marionette server isn't meant to run.

tested this locally and it won't drop the file unless the marionette-server is supposed to start via --marionette cli arg or by the pref.

Comment 2

[Jonathan Griffin (:jgriffin)]

Comment on attachment 807809 [details] [diff] [review]
remove log lines when marionette is not going to be running

Review of attachment 807809 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Why does marionette log to file at all?

Comment 4

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #3)
> Why does marionette log to file at all?

We can remove the file now too, since no one is using it anymore.

So if it's not a debug build (or a b2g build), do we want to have any log output? If we remove the file logging, then we won't have any stdout (dump appenders) for any build other than these two build types. Should we output to stdout for any type of build? I'm not sure what we want here.

Comment 5

[Jim Jeffery not reading bug-mail 1/2/11]

Can this also be back-ported to Firefox 25, 26 ?

Comment 6

[Jonathan Griffin (:jgriffin)]

(In reply to Malini Das [:mdas] from comment #4)
> (In reply to :Gavin Sharp (use [e-mail] for email) from comment
> #3)
> > Why does marionette log to file at all?
> 
> We can remove the file now too, since no one is using it anymore.
> 
> So if it's not a debug build (or a b2g build), do we want to have any log
> output? If we remove the file logging, then we won't have any stdout (dump
> appenders) for any build other than these two build types. Should we output
> to stdout for any type of build? I'm not sure what we want here.

We should probably default to not dumping anything on opt Firefox builds, but have a preference that can be set to enable this, so that we can still get meaningful debug information from TBPL jobs.  We can set that preference in http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/geckoinstance.py.

Comment 7

[Jonathan Griffin (:jgriffin)]

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #5)
> Can this also be back-ported to Firefox 25, 26 ?

Yes, if this is useful we can back-port it.

Comment 8

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: remove marionette.log file, and enable stdout logging via pref for non-debug/non-b2g builds

Tested locally for debug/non-debug builds.

This adds support for a pref called 'marionette.logging' which controls marionette logging for non-debug/non-b2g builds.

Comment 9

[Jonathan Griffin (:jgriffin)]

Comment on attachment 823480 [details] [diff] [review]
remove marionette.log file, and enable stdout logging via pref for non-debug/non-b2g builds

Review of attachment 823480 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/components/marionettecomponent.js
@@ +38,5 @@
>  #endif
>  #ifdef MOZ_B2G
>    dumper = true;
>  #endif
> +  if (dumper || Services.prefs.getBoolPref(MARIONETTE_LOG_PREF)) {

getBoolPref will throw an exception when this pref doesn't exist, so it needs to be inside a try/catch block.

Comment 10

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: remove marionette.log file, and enable stdout logging via pref for non-debug/non-b2g builds v2.0

Thanks, I didn't get any error output in the minimal gecko log in the non-debug build so I didn't realize that was an issue. Fixed in this patch and tested.

Comment 11

[Jonathan Griffin (:jgriffin)]

Comment on attachment 823587 [details] [diff] [review]
remove marionette.log file, and enable stdout logging via pref for non-debug/non-b2g builds v2.0

Review of attachment 823587 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

::: testing/marionette/components/marionettecomponent.js
@@ +42,5 @@
> +  try {
> +    if (dumper || Services.prefs.getBoolPref(MARIONETTE_LOG_PREF)) {
> +      let formatter = new Log.BasicFormatter();
> +      this.logger.addAppender(new Log.DumpAppender(formatter));
> +      this.logger.info("MDAS: bool pref: " + Services.prefs.getBoolPref(MARIONETTE_LOG_PREF));

Can delete this line :)

Comment 12

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: remove marionette.log file, and enable stdout logging via pref for non-debug/non-b2g builds v2.1

Hah whoops. Carrying r+ forward, and testing out on try first before landing:

https://tbpl.mozilla.org/?tree=Try&rev=5e5ab351669a

Comment 13

[Malini Das [:mdas] - Away, not checking bugmail]

looked good on try, pushing to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9d5e3bcc6b

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8a9d5e3bcc6b

Comment 15

[Ryan VanderMeulen [:RyanVM]]

This doesn't apply very well to b2g26 due to bug 451283 landing on Gecko 27. Please post a branch patch :)

Comment 16

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: patch for mozilla-b2g26

This patch should apply cleanly to mozilla-b2g26

Comment 17

[Jim Jeffery not reading bug-mail 1/2/11]

Not sure from this patch if the intention was to not create the log.file at all for end-users, or.. to remove the marionette.log.  

In my test profiles after this landed the marionette.log file remains in my profiles. 
Manually removing them works and they are not re-created... 

At any rate it may not be a big deal to have it linger around, but could perhaps raise questions again down the road when someone else discovers the file.

Comment 18

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #17)
> Not sure from this patch if the intention was to not create the log.file at
> all for end-users, or.. to remove the marionette.log.  
> 
> In my test profiles after this landed the marionette.log file remains in my
> profiles. 
> Manually removing them works and they are not re-created... 

This was to prevent the marionette.log file from being written to a file, and does not remove existing marionette.log files.

> 
> At any rate it may not be a big deal to have it linger around, but could
> perhaps raise questions again down the road when someone else discovers the
> file.
This is a good point though...

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/37134ceb53fc