bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

this.mainViewNode is null error is causing major test failures on mozilla-beta and Cedar

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: mconley, Assigned: adw)

Tracking

unspecified
Firefox 57
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox56+ fixed, firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Here's a stack:

[task 2017-08-01T16:03:50.580286Z] 16:03:50     INFO - Console message: [JavaScript Error: "TypeError: this.mainViewNode is null" {file: "chrome://browser/content/browser-pageActions.js" line: 43}]
[task 2017-08-01T16:03:50.583316Z] 16:03:50     INFO - get mainViewBodyNode@chrome://browser/content/browser-pageActions.js:43:5
[task 2017-08-01T16:03:50.586298Z] 16:03:50     INFO - placeActionInPanel@chrome://browser/content/browser-pageActions.js:98:7
[task 2017-08-01T16:03:50.590435Z] 16:03:50     INFO - placeAction@chrome://browser/content/browser-pageActions.js:72:5
[task 2017-08-01T16:03:50.592522Z] 16:03:50     INFO - init@chrome://browser/content/browser-pageActions.js:51:7
[task 2017-08-01T16:03:50.594536Z] 16:03:50     INFO - onLoad@chrome://browser/content/browser.js:1393:5
[task 2017-08-01T16:03:50.596625Z] 16:03:50     INFO - onload@chrome://browser/content/browser.xul:1:1
[task 2017-08-01T16:03:50.598555Z] 16:03:50     INFO - 
[task 2017-08-01T16:03:50.602123Z] 16:03:50     INFO - Buffered messages logged at 16:02:19
[task 2017-08-01T16:03:50.604483Z] 16:03:50     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 1
[task 2017-08-01T16:03:50.606428Z] 16:03:50     INFO - Buffered messages finished
[task 2017-08-01T16:03:50.608860Z] 16:03:50     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/captivePortal/browser_CaptivePortalWatcher.js | Test timed out - 

I suspect that this is because we're initting the BrowserPageActions object regardless of whether or not Photon is enabled.

We should probably put the initialization of BrowserPageActions behind either a build-time or run-time conditional.
(Assignee)

Updated

a year ago
Assignee: nobody → adw
Status: NEW → ASSIGNED
For sheriffs - if this doesn't make the merge in time, I'm pretty sure bug 1374477 can be backed out on the busted beta and that'll fix this.
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
This should do it.  Doing a run-time check seems to be what other browser-* scripts do.

This made me realize that my fix to the Pocket bug 1367927 assumed that Photon is enabled.  So that will probably break Cedar too?  (If not, it probably will once this patch here lands, since the patch bails out of PageActions.init, which will probably screw up Pockets' PageActions.addAction call, but I'm not sure.)

Gijs, what do you think the strategy should be for PageActions consumers to determine whether page actions are available?  I'm thinking they should check AppConstants.MOZ_PHOTON_THEME before attempting to use PageActions.jsm.  If they don't, then they'll break, or at least their actions won't show up.  Alternatively we could just not bundle PageActions.jsm at build-time.  That way consumers would at least get an error when trying to load the jsm.

But Photon not being enabled is just a temporary thing, right?  So it's not worth coming up with the consumer-friendliest solution IMO.
(Assignee)

Comment 4

a year ago
I filed bug 1386474 for Pocket and use AppConstants.MOZ_PHOTON_THEME, which makes that bug very simple.
Comment hidden (mozreview-request)
(Assignee)

Comment 7

a year ago
The browser_PageActions.js test needs to run on Nightly only.  (There doesn't seem to be a Photon run-if/skip-if?)  The other two page actions-related tests, browser_page_action_menu.js and browser_page_action_menu_clipboard.js, already have a `run-if nightly_build`.
(Assignee)

Comment 9

a year ago
I'm trying to push this patch to try with Photon configure'ed off.  I screwed it up with the last try push, looks like.  Trying again with all "photon" things defaulted to false.

Is there some other way to test this on Cedar?  Other than landing it.
(In reply to Drew Willcoxon :adw from comment #9)
> I'm trying to push this patch to try with Photon configure'ed off.  I
> screwed it up with the last try push, looks like.  Trying again with all
> "photon" things defaulted to false.
> 
> Is there some other way to test this on Cedar?  Other than landing it.

You can push Cedar to try.
(In reply to Mike Conley (:mconley) - Buried in needinfo / review backlog, eating my way out from comment #10)
> You can push Cedar to try.

(I happen to have a Cedar tree cloned locally - if you'd rather I push it to try for you, needinfo me, and I'll do it when I wake up tomorrow)
(Assignee)

Comment 12

a year ago
Ah yeah, thanks Mike.  I'll try that.
(Assignee)

Comment 13

a year ago
Just realized the bookmark star button is ifdef'ed out of browser.xul when building without Photon.
(Assignee)

Comment 15

a year ago
That's a Cedar push to try with the patch.

Not sure whether comment 13 is right/relevant.  Does CUI include the bookmark menu panel button when Photon is not enabled?  Guess I'll find out.

Comment 16

a year ago
mozreview-review
Comment on attachment 8892710 [details]
Bug 1386337 - Disable Photon page actions when Photon is not enabled.

https://reviewboard.mozilla.org/r/163700/#review169126

rs=me. Note that this doesn't fix the browser_pageActions.js test on the trypush or, I expect, on cedar, but it'll work on beta.
Attachment #8892710 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 17

a year ago
Triggered autoland, but bug 1386528 is keeping the tree closed.

Comment 18

a year ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/84230e88dfef
Disable Photon page actions when Photon is not enabled. r=Gijs
[Tracking Requested - why for this release]:

This puts some Photon-specific things (the Page Actions code) behind some conditionals so that it doesn't run (and throw exceptions) when various DOM nodes it wants to work with aren't there.

Without this, the browser load event handler will, I believe, throw an exception, and a bunch of stuff won't get enabled and the browser will be pretty busted.
status-firefox56: --- → affected
tracking-firefox56: --- → ?
Comment on attachment 8892710 [details]
Bug 1386337 - Disable Photon page actions when Photon is not enabled.

Approval Request Comment
[Feature/Bug causing the regression]:

Photon stuff that was working on Nightly, but is disabled / not built on Beta. Specifically, bug 1374477.

[User impact if declined]:

Probably loads of stuff in the browser just won't work - at least, according to the oranges on beta.

[Is this code covered by automated tests?]:

It sure is, that's why beta is currently as orange as a carrot.

[Has the fix been verified in Nightly?]:

adw pushed to try on Cedar (which is the branch that simulates Photon being disabled) and it came back looking good.

[Needs manual test from QE? If yes, steps to reproduce]: 

None.

[List of other uplifts needed for the feature/fix]:

None.

[Is the change risky?]:

No.

[Why is the change risky/not risky?]:

We're just making sure that some Photon stuff is a no-op on 56, which doesn't ship with Photon-y things.

[String changes made/needed]:

None.
Attachment #8892710 - Flags: approval-mozilla-beta?
Summary: this.mainViewNode is null error is causing major test failures on Cedar → this.mainViewNode is null error is causing major test failures on mozilla-beta and Cedar

Comment 21

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/84230e88dfef
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Track 56+ as test failures on mozilla-beta.
tracking-firefox56: ? → +
Comment on attachment 8892710 [details]
Bug 1386337 - Disable Photon page actions when Photon is not enabled.

Anti-carrot squad, please uplift to beta.
Attachment #8892710 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 24

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/4a72eaa119b8
status-firefox56: affected → fixed

Comment 25

11 months ago
59 failures in 888 pushes (0.066 failures/push) were associated with this bug in the last 7 days. 

This is the #27 most frequent failure this week.  

** This failure happened more than 30 times this week! Resolving this bug is a high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 2 weeks, the affected test(s) may be disabled. ** 

Repository breakdown:
* mozilla-beta: 59

Platform breakdown:
* linux64: 27
* android-4-0-armv7-api15: 12
* windowsxp: 5
* windowsxp-devedition: 2
* windows8-64: 2
* linux32: 2
* android-api-15-gradle: 2
* android-4-2-x86: 2
* windows8-64-devedition: 1
* osx-cross-add-on-devel: 1
* osx-cross: 1
* linux64-add-on-devel: 1
* android-5-0-aarch64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1386337&startday=2017-07-31&endday=2017-08-06&tree=all
(In reply to Mike Conley (:mconley) - Holiday until August 8th from comment #20)
> [Is this code covered by automated tests?]:
> 
> It sure is, that's why beta is currently as orange as a carrot.
> 
> [Has the fix been verified in Nightly?]:
> 
> adw pushed to try on Cedar (which is the branch that simulates Photon being
> disabled) and it came back looking good.
> 
> [Needs manual test from QE? If yes, steps to reproduce]: 
> 
> None.

Setting qe-verify- based on Mike's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.