Closed Bug 1384969 Opened 2 years ago Closed 2 years ago

Remove browser.dom.window.dump.enabled from geckodriver recommended prefs

Categories

(Testing :: geckodriver, enhancement)

Version 3
enhancement
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX
mozilla56

People

(Reporter: ato, Assigned: ato)

Details

Attachments

(1 file)

browser.dom.window.dump.enabled causes the browser console messages to reach stdout, often confusing geckodriver users.

We don’t recommend this preference in the Marionette server, although it is set in geckoinstance.py.  This seems appropriate to the Gecko developer audience.

Use of the dump() statement will of course still be possible in chrome level code.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Comment on attachment 8890907 [details]
Bug 1384969 - Stop forwarding browser console output to stdout with geckodriver;

https://reviewboard.mozilla.org/r/162114/#review167384
Attachment #8890907 - Flags: review+
Comment on attachment 8890907 [details]
Bug 1384969 - Stop forwarding browser console output to stdout with geckodriver;

https://reviewboard.mozilla.org/r/162114/#review167392
Attachment #8890907 - Flags: review?(dburns) → review+
hg error in cmd: hg pull gecko -r e71deeb906048f47058f3ff5ef563470006150fe: pulling from https://reviewboard-hg.mozilla.org/gecko
abort: HTTP Error 500: Internal Server Error
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/85b4584e1a83
Stop forwarding browser console output to stdout with geckodriver; r=automatedtester,jgraham
https://hg.mozilla.org/mozilla-central/rev/85b4584e1a83
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
This change was a very bad idea given that it completely stops any debug and trace output from Marionette too. We should get this backed out immediately and ensure it does not hit the next geckodriver release.

David, are you ok in reopening this bug and getting the patch backed out from both central and beta? We should then look more into how we can stop the dump output from the different Firefox components.
Flags: needinfo?(dburns)
Flags: needinfo?(dburns)
We discussed on IRC and the outcome is that we want a backout from central and beta. Thanks.

The underlying problem here is that we are using the DumpAppender for our Marionette logging, which itself is using dump to send the output to stdout. Removing the preference browser.dom.window.dump.enabled will stop any dump output on Windows.

https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Log.jsm#731
Flags: needinfo?(sheriffs)
Carsten or Sebastian, can one of you take care of the backout as requested in comment 8? Thanks.
Flags: needinfo?(sheriffs)
Flags: needinfo?(cbook)
Flags: needinfo?(aryx.bugmail)
Thank you Sebastian!

Given that this is the wrong way doing it I filed bug 1389049 for a better approach.
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → WONTFIX
(In reply to Henrik Skupin (:whimboo) from comment #7)

> This change was a very bad idea given that it completely stops
> any debug and trace output from Marionette too. We should get
> this backed out immediately and ensure it does not hit the next
> geckodriver release.
> 
> David, are you ok in reopening this bug and getting the patch
> backed out from both central and beta? We should then look more
> into how we can stop the dump output from the different Firefox
> components.

I don’t think browser.dom.window.dump.enabled causes dump() itself
to stop working in chrome space?  The intention here is to stop web
content browser console warnings from being forwarded to stdout.

Also worth noting, the preference is still being used in
geckoinstance.py because it is useful in the context of working on
the web browser itself.
Flags: needinfo?(hskupin)
(In reply to Henrik Skupin (:whimboo) from comment #8)

> The underlying problem here is that we are using the DumpAppender
> for our Marionette logging, which itself is using dump
> to send the output to stdout.  Removing the preference
> browser.dom.window.dump.enabled will stop any dump output on
> Windows.

This is something else entirely tracked in
https://bugzil.la/1384956, which is possibly about remote browsers
and the use of Log.jsm.
(In reply to Andreas Tolfsen ‹:ato› from comment #12)
> I don’t think browser.dom.window.dump.enabled causes dump() itself
> to stop working in chrome space?  The intention here is to stop web
> content browser console warnings from being forwarded to stdout.

Why do you think so? It's exactly what this preference stands for:
https://developer.mozilla.org/en-US/docs/Web/API/Window/dump

Which says:

> In Gecko dump() is disabled by default – it doesn't do anything but doesn't raise an error either. To see the dump output you
> have to enable it by setting the preference browser.dom.window.dump.enabled to true. You can set the preference in about:config
> or in a user.js file.
Flags: needinfo?(hskupin)
(In reply to Andreas Tolfsen ‹:ato› from comment #13)
> (In reply to Henrik Skupin (:whimboo) from comment #8)
> 
> > The underlying problem here is that we are using the DumpAppender
> > for our Marionette logging, which itself is using dump
> > to send the output to stdout.  Removing the preference
> > browser.dom.window.dump.enabled will stop any dump output on
> > Windows.
> 
> This is something else entirely tracked in
> https://bugzil.la/1384956, which is possibly about remote browsers
> and the use of Log.jsm.

No, it's not because of the usage of dump().
You need to log in before you can comment on or make changes to this bug.