Closed Bug 1490412 Opened 2 years ago Closed 2 years ago

browser.dom.window.dump.enabled is false by default in local artifact builds

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(1 file)

`./mach run --temp-profile about:config` and searching for "browser.dom.window.dump.enabled", I see false in a local build in m-c.

From Bug 1489844 (https://hg.mozilla.org/mozilla-central/rev/2d219572a965#l4.12), it looks like the pref values got translated from:

-#ifdef MOZILLA_OFFICIAL
-// enable JS dump() function.
-pref("browser.dom.window.dump.enabled", false, sticky);
-#else
-pref("browser.dom.window.dump.enabled", true, sticky);
-#endif

To 

+#ifdef MOZILLA_OFFICIAL
+# define PREF_VALUE false
+#else
+# define PREF_VALUE true
+#endif
+VARCACHE_PREF(
+  "browser.dom.window.dump.enabled",
+   browser_dom_window_dump_enabled,
+  RelaxedAtomicBool, PREF_VALUE
+)
+#undef PREF_VALUE

Which seems fine, so I'm not sure what broke here.
Flags: needinfo?(amarchesini)
I think this is artifact build related. Previously the pref was set in all.js which would get build based on the value of MOZILLA_OFFICIAL locally (0), but now it's being built the with value from automation (1).
Summary: browser.dom.window.dump.enabled is false by default in local builds → browser.dom.window.dump.enabled is false by default in local artifact builds
Andrea, is it possible to move "browser.dom.window.dump.enabled" back to all.js (or override in firefox.js) to restore the old behavior in artifact builds? Or does it have to be declared in C++ for DOMPrefs to be removed? 

Some other options I've thought of:
1) Set the pref in `./mach run` for the default user (as we do for default browser check https://searchfox.org/mozilla-central/rev/9e7995b3c384945740985107a4de601b27904718/python/mozbuild/mozbuild/mach_commands.py#916). Downside: that wouldn't apply if you either run the binary directly or run it with a custom profile
2) Have some JS run at startup and flip the pref based on AppConstants.MOZILLA_OFFICIAL. Downside: if you then went and turned off the pref it would get turned back on again at the next startup.
3) Change the automation to not build with MOZILLA_OFFICIAL on pushes to m-c so the binary is consistent with the build variables locally. This could also help with some issues with comparing screenshots on try vs an m-c base, but I assume this would be non-trivial and I know the least about the downsides here.

I'd like to get a fix for this ASAP since it affects frontend development workflow (no console.log or dump statements are showing up in stdout).
Oh and:

(4) Change the check from MOZILLA_OFFICIAL to `MOZ_UPDATE_CHANNEL == "default"` (or maybe `MOZ_UPDATE_CHANNEL == "default" || MOZ_UPDATE_CHANNEL == "nightly"` depending on what the update channel is on these m-c pushes). This may change the behavior (enabling dump by default in nightly), but that could be OK as long as we don't enable it on Beta/Release.
(In reply to Brian Grinstead [:bgrins] from comment #3)
> Oh and:
> 
> (4) Change the check from MOZILLA_OFFICIAL to `MOZ_UPDATE_CHANNEL ==
> "default"` (or maybe `MOZ_UPDATE_CHANNEL == "default" || MOZ_UPDATE_CHANNEL
> == "nightly"` depending on what the update channel is on these m-c pushes).
> This may change the behavior (enabling dump by default in nightly), but that
> could be OK as long as we don't enable it on Beta/Release.

If I download a build from m-c (like https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&selectedJob=198609857), and run with the Browser Console: AppConstants.MOZ_UPDATE_CHANNEL then I get "default". I'm just not sure if MOZ_UPDATE_CHANNEL is a reliable way to detect local builds.

Maybe NIGHTLY_BUILD would be better? `AppConstants.NIGHTLY_BUILD` also is true on that m-c binary as well as my local artifact build.
MOZ_OFFICIAL is true for binaries fetched in artifact builds, so
browser.dom.window.dump.enabled is false. This isn't intended behavior -
local builds should have dumping enabled by default.

Changing to NIGHTLY_BUILD fixes this. It also enables dump by
default for official Nightly builds, but as long as it's not on in
release or beta that's probably OK.
Attachment #9008193 - Attachment description: Bug 1490412 - Use NIGHTLY_BUILD instead of MOZ_OFFICIAL for dumping to stdout → Bug 1490412 - Use NIGHTLY_BUILD instead of !MOZILLA_OFFICIAL for dumping to stdout
I believe this would also enable dump by default for official Nightly builds which is a web-exposed behavior change (see https://developer.mozilla.org/en-US/docs/Web/API/Window/dump), so we'd have to decide if that's acceptable. Mossop, what do you think?
Flags: needinfo?(dtownsend)
(In reply to Brian Grinstead [:bgrins] from comment #2)
> Andrea, is it possible to move "browser.dom.window.dump.enabled" back to
> all.js (or override in firefox.js) to restore the old behavior in artifact
> builds? Or does it have to be declared in C++ for DOMPrefs to be removed? 

Please add the removed definition back to all.js.  No need to touch anything on the C++ side.  That should fix this bug.  r=me on the patch that does this.  :-)
Attachment #9008193 - Attachment description: Bug 1490412 - Use NIGHTLY_BUILD instead of !MOZILLA_OFFICIAL for dumping to stdout → Bug 1490412 - Revert the part of Bug 1489844 that removed browser.dom.window.dump.enabled from all.js;r=Ehsan
(In reply to :Ehsan Akhgari from comment #7)
> (In reply to Brian Grinstead [:bgrins] from comment #2)
> > Andrea, is it possible to move "browser.dom.window.dump.enabled" back to
> > all.js (or override in firefox.js) to restore the old behavior in artifact
> > builds? Or does it have to be declared in C++ for DOMPrefs to be removed? 
> 
> Please add the removed definition back to all.js.  No need to touch anything
> on the C++ side.  That should fix this bug.  r=me on the patch that does
> this.  :-)

Thanks, that will be a better option. I guess we'll need to make sure that the condition in all.js and StaticPrefList.h stay in sync going forward? Also, does the fact that it's a sticky pref in all.js cause any issue with the way StaticPrefs work?
Flags: needinfo?(ehsan)
Flags: needinfo?(dtownsend)
Flags: needinfo?(amarchesini)
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
(In reply to Brian Grinstead [:bgrins] from comment #8)
> (In reply to :Ehsan Akhgari from comment #7)
> > (In reply to Brian Grinstead [:bgrins] from comment #2)
> > > Andrea, is it possible to move "browser.dom.window.dump.enabled" back to
> > > all.js (or override in firefox.js) to restore the old behavior in artifact
> > > builds? Or does it have to be declared in C++ for DOMPrefs to be removed? 
> > 
> > Please add the removed definition back to all.js.  No need to touch anything
> > on the C++ side.  That should fix this bug.  r=me on the patch that does
> > this.  :-)
> 
> Thanks, that will be a better option. I guess we'll need to make sure that
> the condition in all.js and StaticPrefList.h stay in sync going forward?

Yes.  I'd appreciate if you added comments to both places to that effect.

Also I'd appreciate if you added additional comments to the all.js site explaining that it is needed to keep that bit for the purpose of keeping the pref working on artifact builds, since that's far from obvious, for the benefit of the next person to stumble across this.  Thanks!!

> Also, does the fact that it's a sticky pref in all.js cause any issue with
> the way StaticPrefs work?

Not that I'm aware of.  AFAICT it would just override the default provided in StaticPrefs which is fine here.

The core of the issue at hand is one of the core assumptions that the artifact build "hack" depends on, which is, the binaries that we build in automation are the same as the ones we build locally.  That's of course only true mostly, but not quite completely.  One of the biggest differences is whether MOZ_OFFICIAL is defined or not, so all that the all.js code is doing is give us something in the local portion of the artifact build process to override the "mistake" of replacing the binaries we build in infra with the ones we should have built locally.  I this that's an acceptable fix for the issue at hand, and we don't really need to do something more drastic, although the purist in me would be a bit sad that this pref will have two sources of truth for its default value in the tree...
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari from comment #9)
> I this that's an acceptable fix for the
> issue at hand, and we don't really need to do something more drastic,
> although the purist in me would be a bit sad that this pref will have two
> sources of truth for its default value in the tree...

OK, I'll add comments to that effect. Though if we have the pref defined in all.js, does it really matter what the default value provided in StaticPrefList is? Maybe we could just make it false unconditionally there and have all.js be the source of truth for the default value.
Comment on attachment 9008193 [details]
Bug 1490412 - Revert the part of Bug 1489844 that removed browser.dom.window.dump.enabled from all.js;r=Ehsan

:Ehsan Akhgari has approved the revision.
Attachment #9008193 - Flags: review+
Alright thanks, added a comment. Before pushing, I'd just like to make sure that we can't remove the duplication by unconditionally setting to false in StaticPrefList (see Comment 10).
Flags: needinfo?(ehsan)
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/99de4ec496f4
Revert the part of Bug 1489844 that removed browser.dom.window.dump.enabled from all.js;r=Ehsan
(In reply to Brian Grinstead [:bgrins] from comment #12)
> Alright thanks, added a comment. Before pushing, I'd just like to make sure
> that we can't remove the duplication by unconditionally setting to false in
> StaticPrefList (see Comment 10).

Ah sorry I didn't see this in time.  I see what you mean.  To be honest, I don't know which one takes precedence.  :-)  It's easy to test if you care enough, but your patch is fine as is, IMO.
Flags: needinfo?(ehsan)
https://hg.mozilla.org/mozilla-central/rev/99de4ec496f4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.