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

Status: RESOLVED FIXED

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

Description

[Brian Grinstead [:bgrins]]

`./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.

Comment 1

[Brian Grinstead [:bgrins]]

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).

Comment 2

[Brian Grinstead [:bgrins]]

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).

Comment 3

[Brian Grinstead [:bgrins]]

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.

Comment 4

[Brian Grinstead [:bgrins]]

(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.

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: Bug 1490412 - Revert the part of Bug 1489844 that removed browser.dom.window.dump.enabled from all.js;r=Ehsan

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.

Comment 6

[Brian Grinstead [:bgrins]]

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?

Comment 7

[(no longer active)]

(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.  :-)

Comment 8

[Brian Grinstead [:bgrins]]

(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?

Comment 9

[(no longer active)]

(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...

Comment 10

[Brian Grinstead [:bgrins]]

(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 11

[(no longer active)]

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.

Comment 12

[Brian Grinstead [:bgrins]]

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).

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[(no longer active)]

(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.

Comment 15

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/99de4ec496f4