Closed Bug 1317317 Opened 8 years ago Closed 8 years ago

Aurora Permafailure in browser_newtabButton.js | Uncaught exception - at resource://testing-common/BrowserTestUtils.jsm:609 - TypeError: subject is null

Categories

(Firefox :: Tabbed Browser, defect, P2)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: cbook, Assigned: jkt)

References

(Blocks 2 open bugs,
URL
)

Details

(Whiteboard: [userContextId])

Attachments

(1 file)

Bug 1317317 - Await for popup menu to exist for container long press button
58 bytes, text/x-review-board-request
Gijs
: review+
jcristau
: approval-mozilla-aurora+
Details 
regression from Bug 1272256 

https://treeherder.mozilla.org/logviewer.html#?job_id=4170781&repo=mozilla-aurora

blocks reopening aurora after the merge day
Disabled, as this test is only testing behaviour when the pref is flipped.

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/ebec82f380befccdb9653a607330835cd5f7b1be

Jonathan, you said some API changed inbetween when you wrote the patch on 1272256 and when it landed. Can you elaborate?
Flags: needinfo?(jkt)
Keywords: leave-open
It's certainly not the API change I thought it was, it just uncannily the exact same error message as something else. I'm still building after a clobber but it loks to me more like a timing issue with flipping the pref and the elements existing.

Will update when I have more info.
Blocks: ContextualIdentity
Flags: needinfo?(jkt)
Assignee: nobody → jkt
Priority: -- → P2
Whiteboard: [userContextId]
Comment on attachment 8810973 [details]
Bug 1317317 - Await for popup menu to exist for container long press button

https://reviewboard.mozilla.org/r/93238/#review93234

I'm assuming you tested this against aurora with the test re-enabled? If so, please uplift this there as well (you don't need explicit approval for a test fix generally, also in this case because we explicitly disabled this for the sake of reopening the tree quickly, so it seems fair to then re-enable ASAP when we have a fix).

::: browser/components/contextualidentity/test/browser/browser_newtabButton.js:15
(Diff revision 1)
>  
>    let newTab = document.getElementById('tabbrowser-tabs');
>    let newTabButton = document.getAnonymousElementByAttribute(newTab, "anonid", "tabs-newtab-button");
>    ok(newTabButton, "New tab button exists");
>    ok(!newTabButton.hidden, "New tab button is visible");
> +  yield BrowserTestUtils.waitForCondition(() => !!document.getAnonymousElementByAttribute(newTab, "anonid", "newtab-popup"), 'Wait for popup to exist');

Nit: double quotes for the message.
Attachment #8810973 - Flags: review?(gijskruitbosch+bugs) → review+
No it doesn't pass on Aurora, I was testing central. Will look into thanks!
Gijs,

So the fix is actually: https://reviewboard.mozilla.org/r/93238/diff/1-2/ which required a pref flip in nightly, restart and retry where as in Aurora it's not enabled by default so this autofailed.

Could I confirm the r+ still as this is actually changing code, I fixed the nit above too (doesn't harm to keep the check in there for the button to combat race conditions in the test).

This should be put onto central and uplifted to aurora right?

Thanks!
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Jonathan Kingston [:jkt] from comment #12)
> Gijs,

NB: I'm on PTO. Further replies on Monday, if that's OK. :-)

> So the fix is actually: https://reviewboard.mozilla.org/r/93238/diff/1-2/
> which required a pref flip in nightly, restart and retry where as in Aurora
> it's not enabled by default so this autofailed.
> 
> Could I confirm the r+ still as this is actually changing code, I fixed the
> nit above too (doesn't harm to keep the check in there for the button to
> combat race conditions in the test).

Well, it's a bit confusing - the code in question should be firing when the pref is turned *off*, not when it's turned *on*. How is it breaking this on aurora? I wouldn't have expected the 'turn off' code to fire there *before* the test runs at all?

> This should be put onto central and uplifted to aurora right?
Yes.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jkt)
I'm on PTO/travelling till late Tuesday anyway :).

The reason is on Aurora the code path causes an exception which means it never continues to monitoring the pref when it is turned on. Nightly does the same if you change the pref to off then on again. However after a restart with the pref enabled the browser does the right thing.
Flags: needinfo?(jkt) → needinfo?(gijskruitbosch+bugs)
Blocks: 1298064
(In reply to Jonathan Kingston [:jkt] from comment #14)
> I'm on PTO/travelling till late Tuesday anyway :).
> 
> The reason is on Aurora the code path causes an exception which means it
> never continues to monitoring the pref when it is turned on. Nightly does
> the same if you change the pref to off then on again. However after a
> restart with the pref enabled the browser does the right thing.

I'm not following, I'm afraid.

On aurora, the pref starts as 'off'. The codepath in question is for when the pref flips from 'on' to 'off'. I would not expect that to happen in the test on aurora until after the test finishes, but the logs show it happening mid-test. What's going on there?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jkt)
So the code gets initialized with the code above:
  this.observe(null, "nsPref:changed", "privacy.userContext.enabled");
and also maintains state with:
  Services.prefs.addObserver("privacy.userContext.enabled", this, false);

Aurora initializes the browser with the else clause before the tests even start which is what triggers the exception. By the time the code comes to enable it the code won't continue to run.

So the code fails at my new 'wait for popup to exist' as the code inside the enabled branch 'if (containersEnabled) {' never fires due to the exception so the popup never exists.
Flags: needinfo?(jkt) → needinfo?(gijskruitbosch+bugs)
(In reply to Jonathan Kingston [:jkt] from comment #16)
> So the code gets initialized with the code above:
>   this.observe(null, "nsPref:changed", "privacy.userContext.enabled");

Oh, we explicitly init with this. Right, that explains it.
Flags: needinfo?(gijskruitbosch+bugs)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c4afd11b52ab
Await for popup menu to exist for container long press button r=Gijs
Was that autoland for central? We need uplift to Aurora too right?
Thanks!
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Jonathan Kingston [:jkt] from comment #19)
> Was that autoland for central? We need uplift to Aurora too right?
> Thanks!

Yes, but we should wait until this makes m-c - should be happening in the next 20-odd hours or so.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8810973 [details]
Bug 1317317 - Await for popup menu to exist for container long press button

Approval Request Comment
[Feature/regressing bug #]: 1272256
[User impact if declined]: Tests are failing on Aurora
[Describe test coverage new/current, TreeHerder]: Improved failure information when things break.
[Risks and why]: Should be no risks as now skipping on an unknown element which was causing a failure on start in Aurora.
[String/UUID change made/needed]:
Attachment #8810973 - Flags: approval-mozilla-aurora?
Comment on attachment 8810973 [details]
Bug 1317317 - Await for popup menu to exist for container long press button

newtab-popup test fix, take in aurora52

Jonathan, was the leave-open flag intentionally kept on this bug, or can this be closed now?
Flags: needinfo?(jkt)
Attachment #8810973 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → affected
status-firefox53: --- → fixed
Flags: needinfo?(jkt)
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: