1368370 - Permaorange on DevEdition and beta in browser_photon_customization_context_menus.js when Gecko 55 merges to beta on 2016-06-12

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Phil Ringnalda (:philor)]

https://treeherder.mozilla.org/logviewer.html#?job_id=102697503&repo=try

Guessing that the test isn't expecting the developer toolbar button?

Minimum to repro locally is probably changing /config/milestone.txt from 55.0a1 to 55.0 and building with ac_add_options --with-branding=browser/branding/aurora (and clobbering, and not trying to do an artifact build). Try with all three platforms is https://hg.mozilla.org/try/rev/fa9d0b5989246f52b218e875871e2bb129c2316b plus -p linux-devedition-nightly,linux64-devedition-nightly,macosx64,win32,win64

[Tracking Requested - why for this release]: merge bustage, closed tree, delayed b1.

Comment 1

[Phil Ringnalda (:philor)]

Oh, not just DevEdition, didn't realize I was pushing a different revision of m-c than my last central-as-beta. Same timeout on opt, add in some leaked until shutdown noise for debug, https://treeherder.mozilla.org/logviewer.html#?job_id=102716347&repo=try

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1368370 - remove leftover page action hiding,

Review commit: https://reviewboard.mozilla.org/r/143818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/143818/

Comment 3

[:Gijs (he/him)]

The test is the first that actively flips the photon structure pref, and it's paying for that by tripping up code that should have been removed in bug 1364090 but wasn't (even though the hidden=true attribute was removed...).

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8872314 [details]
Bug 1368370 - remove leftover page action hiding,

https://reviewboard.mozilla.org/r/143818/#review147508

I feel like I'm missing some context. Is there some other code hiding the button besides the ifdef? Otherwise the removed code looks like it should be a no-op, so I don't see how this would fix a test.

The patch by itself seems fine regardless.

Comment 5

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #4)
> Comment on attachment 8872314 [details]
> Bug 1368370 - remove leftover page action hiding,
> 
> https://reviewboard.mozilla.org/r/143818/#review147508
> 
> I feel like I'm missing some context. Is there some other code hiding the
> button besides the ifdef? Otherwise the removed code looks like it should be
> a no-op, so I don't see how this would fix a test.
> 
> The patch by itself seems fine regardless.

First link in comment #0:

[task 2017-05-28T17:39:10.578185Z] 17:39:10     INFO - Console message: [JavaScript Error: "TypeError: this.button is null" {file: "chrome://browser/content/browser.js" line: 7747}]
[task 2017-05-28T17:39:10.580046Z] 17:39:10     INFO - init@chrome://browser/content/browser.js:7747:7
[task 2017-05-28T17:39:10.583402Z] 17:39:10     INFO - _delayedStartup@chrome://browser/content/browser.js:1713:5

... because this used to be behind a pref (the code I'm removing, and the button was always in the markup but had hidden=true). Then bug 1364090 removed the hidden attribute but added ifdefs, and didn't remove this code, which runs on window init / delayed startup. So now if we run without the ifdef but *with* the pref, like say on aurora/beta, on a test that checks that other code behind the pref works, and we happen to open a new window... boom.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/79c629f3a50e
remove leftover page action hiding, r=dao

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/79c629f3a50e