1369750 - Intermittent browser/base/content/test/general/browser_aboutHome.js | A promise chain failed to handle a rejection: this._window.document.body is null

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect, P1)

Description

[Treeherder Bug Filer]

Filed by: rvandermeulen [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=104003562&repo=autoland

https://queue.taskcluster.net/v1/task/Z4o-sxA0Sk-4cmB2H0K4Iw/runs/0/artifacts/public/logs/live_backing.log

Comment 1

[Ryan VanderMeulen [:RyanVM]]

The timing of when this started makes me very suspicious of bug 1369296.

Comment 3

[Intermittent Failures Robot]

11 failures in 820 pushes (0.013 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-inbound: 5
* mozilla-central: 3
* autoland: 3

Platform breakdown:
* windows7-32-vm: 6
* linux64: 3
* linux32-nightly: 1
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1369750&startday=2017-05-29&endday=2017-06-04&tree=all

Comment 4

[[e-mail]]

In https://queue.taskcluster.net/v1/task/Z4o-sxA0Sk-4cmB2H0K4Iw/runs/0/artifacts/public/logs/live_backing.log I found

09:21:57     INFO -  GECKO(3112) | JavaScript error: resource://onboarding/onboarding.js, line 32: TypeError: this._window.document.body is null

It means occasionally the `document` or `document.body` in contentWindow object is not available.


I can add some dom exist check before calling `new Onboarding(content);` to detour the Intermittent, but it does not find the root cause.


```
if (!content.document || !content.document.body) {
  return;
}
```

http://searchfox.org/mozilla-central/source/browser/extensions/onboarding/content/onboarding.js#105

Dave, do you know why it happens when we access content after got the `load` event?

Comment 5

[Fischer [:Fischer]]

(In reply to Fred Lin [:gasolin] from comment #4)
> In
> https://queue.taskcluster.net/v1/task/Z4o-sxA0Sk-4cmB2H0K4Iw/runs/0/
> artifacts/public/logs/live_backing.log I found
> 
> 09:21:57     INFO -  GECKO(3112) | JavaScript error:
> resource://onboarding/onboarding.js, line 32: TypeError:
> this._window.document.body is null
> 
> It means occasionally the `document` or `document.body` in contentWindow
> object is not available.
> 
> 
> I can add some dom exist check before calling `new Onboarding(content);` to
> detour the Intermittent, but it does not find the root cause.
> 
> 
> ```
> if (!content.document || !content.document.body) {
>   return;
> }
> ```
> 
> http://searchfox.org/mozilla-central/source/browser/extensions/onboarding/
> content/onboarding.js#105
> 
> Dave, do you know why it happens when we access content after got the `load`
> event?
Maybe we can use the passed-in event object instead, such as,

```
addEventListener("load", function(evt) {
  content.console.log("evt.target =", evt.target); // This is the document
  let window = evt.target.defaultView;
  // Load onboarding module only when we enable it.
  if ((window.location.href == ABOUT_NEWTAB_URL ||
       window.location.href == ABOUT_HOME_URL) &&
      Services.prefs.getBoolPref("browser.onboarding.enabled", false)) {

    window.requestIdleCallback(() => {
      new Onboarding(window);
    });
  }
}, true);
```

Comment 6

[Fischer [:Fischer]]

(In reply to Fischer [:Fischer] from comment #5)
> (In reply to Fred Lin [:gasolin] from comment #4)
> > In
> > https://queue.taskcluster.net/v1/task/Z4o-sxA0Sk-4cmB2H0K4Iw/runs/0/
> > artifacts/public/logs/live_backing.log I found
> > 
> > 09:21:57     INFO -  GECKO(3112) | JavaScript error:
> > resource://onboarding/onboarding.js, line 32: TypeError:
> > this._window.document.body is null
> > 
> > It means occasionally the `document` or `document.body` in contentWindow
> > object is not available.
> > 
> > 
> > I can add some dom exist check before calling `new Onboarding(content);` to
> > detour the Intermittent, but it does not find the root cause.
> > 
> > 
> > ```
> > if (!content.document || !content.document.body) {
> >   return;
> > }
> > ```
> > 
> > http://searchfox.org/mozilla-central/source/browser/extensions/onboarding/
> > content/onboarding.js#105
> > 
> > Dave, do you know why it happens when we access content after got the `load`
> > event?
> Maybe we can use the passed-in event object instead, such as,
> 
> ```
> addEventListener("load", function(evt) {
>   content.console.log("evt.target =", evt.target); // This is the document
>   let window = evt.target.defaultView;
>   // Load onboarding module only when we enable it.
>   if ((window.location.href == ABOUT_NEWTAB_URL ||
>        window.location.href == ABOUT_HOME_URL) &&
>       Services.prefs.getBoolPref("browser.onboarding.enabled", false)) {
> 
>     window.requestIdleCallback(() => {
>       new Onboarding(window);
>     });
>   }
> }, true);
> ```
Running a try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=417950e70c9954378c855287a8a0aead5f23fdf4

Comment 7

[Fischer [:Fischer]]

(In reply to Fischer [:Fischer] from comment #6)
> (In reply to Fischer [:Fischer] from comment #5)
> > Maybe we can use the passed-in event object instead, such as,
> > 
> > ```
> > addEventListener("load", function(evt) {
> >   content.console.log("evt.target =", evt.target); // This is the document
> >   let window = evt.target.defaultView;
> >   // Load onboarding module only when we enable it.
> >   if ((window.location.href == ABOUT_NEWTAB_URL ||
> >        window.location.href == ABOUT_HOME_URL) &&
> >       Services.prefs.getBoolPref("browser.onboarding.enabled", false)) {
> > 
> >     window.requestIdleCallback(() => {
> >       new Onboarding(window);
> >     });
> >   }
> > }, true);
> > ```
> Running a try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=417950e70c9954378c855287a8a0aead5f23fdf4
Stop the previous one(because of some redundant and error-making test logs) and push another TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=929e0351ad11cee5e720ac7236fb055aac593bad

Comment 8

[Fischer [:Fischer]]

Attachment: Bug 1369750 - Get window from event.target.defaultView inside load event to make

sure getting the right target window, r?mossop

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

Comment 9

[Fischer [:Fischer]]

Hi Mossop,
This patch uses window from event.target.defaultView inside load event to make sure getting the right target window.
TRY is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=929e0351ad11cee5e720ac7236fb055aac593bad.

Comment 10

[Dave Townsend [:mossop]]

(In reply to Fred Lin [:gasolin] from comment #4)
> In
> https://queue.taskcluster.net/v1/task/Z4o-sxA0Sk-4cmB2H0K4Iw/runs/0/
> artifacts/public/logs/live_backing.log I found
> 
> 09:21:57     INFO -  GECKO(3112) | JavaScript error:
> resource://onboarding/onboarding.js, line 32: TypeError:
> this._window.document.body is null
> 
> It means occasionally the `document` or `document.body` in contentWindow
> object is not available.
> 
> 
> I can add some dom exist check before calling `new Onboarding(content);` to
> detour the Intermittent, but it does not find the root cause.
> 
> 
> ```
> if (!content.document || !content.document.body) {
>   return;
> }
> ```
> 
> http://searchfox.org/mozilla-central/source/browser/extensions/onboarding/
> content/onboarding.js#105
> 
> Dave, do you know why it happens when we access content after got the `load`
> event?

We might be getting load events for images and other resources inside the new tab page meaning the DOM might not be complete at that point. What we should do is verify that the load event is coming from the content window.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 8874398 [details]
Bug 1369750 - Get window from event.target.defaultView inside load event to make

https://reviewboard.mozilla.org/r/145772/#review149776

I think what we should be doing here is testing that evt.target == content.document. This verifies that the load event is coming from the document for the top-level window.

Comment 12

[Fischer [:Fischer]]

Comment on attachment 8874398 [details]
Bug 1369750 - Get window from event.target.defaultView inside load event to make

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145772/diff/1-2/

Comment 13

[Fischer [:Fischer]]

(In reply to Fischer [:Fischer] from comment #12)
> Comment on attachment 8874398 [details]
> Bug 1369750 - Get window from event.target.defaultView inside load event to
> make
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/145772/diff/1-2/
Hi Mossop,
Thanks for explanation to see the updated patch.

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 8874398 [details]
Bug 1369750 - Get window from event.target.defaultView inside load event to make

https://reviewboard.mozilla.org/r/145772/#review150272

Looks good, thanks for the change.

Comment 15

[Fischer [:Fischer]]

TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eacb37a4fe0b9db3127b9e3b734f661cfc206afd

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57187d8e1433
Get window from event.target.defaultView inside load event to make r=mossop

Comment 17

[Fischer [:Fischer]]

Hi Philor

Comment 18

[Fischer [:Fischer]]

(In reply to Fischer [:Fischer] from comment #17)
> Hi Philor
(Sorry for the accidentally hitting the enter key)
Is this bug ready to land into the mc? thanks

Comment 19

[Phil Ringnalda (:philor)]

After 21 hours without being backed out? I'd certainly think so, yes, but that's not how getting to m-c works. It isn't a matter of "my push is fine," but instead a matter of "a sheriff manages to arrange his workday correctly, and a push exists somewhere above my push which has gotten PGO builds and all tests run on it and which doesn't have any bustage landed below it which wasn't also backed out below it, so that that push and everything which landed before it can be merged."

If instead you mean "why didn't Wes merge anything from autoland to mozilla-central during his workday today?" then I don't know, or if you mean "why did Ryan choose to merge mozilla-inbound to mozilla-central well after his workday even though merging isn't his job, but chose not to merge autoland to mozilla-central?" then I also don't know that.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/57187d8e1433

Comment 21

[Justin [:JW_SoftvisionQA]]

Is there any verification needed from QA on this? If so, what exactly am I looking for when testing this fix? Thanks

Comment 22

[Fischer [:Fischer]]

(In reply to Justin [:JW_SoftvisionQA] from comment #21)
> Is there any verification needed from QA on this? If so, what exactly am I
> looking for when testing this fix? Thanks
During testing, please open the browser toolbox and the developer debugger and check out the console to see if some error about "onboarding". Thank you.

Comment 23

[Justin [:JW_SoftvisionQA]]

I have verified that no errors about onboarding is shown in the browser toolbox or the developer debugger.

Comment 24

[Intermittent Failures Robot]

6 failures in 892 pushes (0.007 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-inbound: 3
* autoland: 3

Platform breakdown:
* windows8-64: 6

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1369750&startday=2017-06-19&endday=2017-06-25&tree=all