1277834 - Intermittent browser_toolbox_window_reload_target.js | A promise chain failed to handle a rejection: - at resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/breadcrumbs.js:757 - TypeError: this.nodeHierarchy[this.cur

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Carsten Book [:Tomcat]]

https://treeherder.mozilla.org/logviewer.html#?job_id=29399375&repo=mozilla-inbound#L2489

 08:04:45 INFO - 209 INFO TEST-UNEXPECTED-FAIL | devtools/client/framework/test/browser_toolbox_window_reload_target.js | A promise chain failed to handle a rejection: - at resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/breadcrumbs.js:757 - TypeError: this.nodeHierarchy[this.currentIndex] is undefined

Comment 1

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 2

[Intermittent Failures Robot]

16 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* fx-team: 6
* mozilla-inbound: 4
* mozilla-central: 4
* try: 2

Platform breakdown:
* linux32: 8
* linux64: 7
* windows8-64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1277834&startday=2016-05-30&endday=2016-06-05&tree=all

Comment 3

[Intermittent Failures Robot]

74 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 42
* fx-team: 19
* mozilla-central: 7
* try: 4
* ash: 2

Platform breakdown:
* linux64: 54
* linux32: 11
* windows7-32: 5
* windowsxp: 2
* osx-10-10: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1277834&startday=2016-06-06&endday=2016-06-12&tree=all

Comment 4

[Intermittent Failures Robot]

19 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 14
* fx-team: 3
* mozilla-central: 1
* ash: 1

Platform breakdown:
* linux32: 9
* linux64: 7
* windows7-32: 3

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

Comment 5

[Intermittent Failures Robot]

50 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 27
* fx-team: 9
* try: 4
* mozilla-central: 3
* mozilla-aurora: 3
* autoland: 2
* ash: 2

Platform breakdown:
* linux64: 32
* linux32: 11
* windows7-32-vm: 4
* windowsxp: 1
* windows7-32: 1
* osx-10-10: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1277834&startday=2016-06-20&endday=2016-06-26&tree=all

Comment 6

[Intermittent Failures Robot]

42 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 12
* autoland: 11
* mozilla-central: 6
* fx-team: 6
* mozilla-aurora: 5
* try: 1
* ash: 1

Platform breakdown:
* linux64: 23
* linux32: 12
* windows7-32-vm: 5
* windowsxp: 1
* osx-10-10: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1277834&startday=2016-06-27&endday=2016-07-03&tree=all

Comment 7

[Intermittent Failures Robot]

18 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 6
* fx-team: 4
* mozilla-central: 3
* mozilla-aurora: 2
* try: 1
* autoland: 1
* ash: 1

Platform breakdown:
* linux64: 16
* windows7-32: 1
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1277834&startday=2016-07-07&endday=2016-07-07&tree=all

Comment 8

[Intermittent Failures Robot]

72 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 24
* autoland: 17
* fx-team: 11
* mozilla-central: 8
* mozilla-aurora: 6
* ash: 4
* try: 2

Platform breakdown:
* linux64: 45
* linux32: 16
* windows7-32-vm: 7
* osx-10-10: 2
* windowsxp: 1
* windows7-32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1277834&startday=2016-07-04&endday=2016-07-10&tree=all

Comment 9

[Intermittent Failures Robot]

54 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 19
* fx-team: 11
* autoland: 11
* mozilla-central: 5
* mozilla-aurora: 5
* try: 2
* ash: 1

Platform breakdown:
* linux64: 29
* linux32: 17
* windows7-32-vm: 5
* osx-10-10: 2
* windowsxp: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1277834&startday=2016-07-11&endday=2016-07-17&tree=all

Comment 10

[Ryan VanderMeulen [:RyanVM]]

This looks pretty frequent. Can we please bump the priority on it? Maybe bug 1241019 while we're at it? :)

Comment 11

[Patrick Brosset <:pbro>]

Thanks Ryan, this should indeed really be a P1.
I had seen this test failing often but thought it was a devtools framework thing (it's a toolbox test), but it's instead an inspector problem, which is definitely on my radar. I'll make sure it gets worked on.

Comment 12

[Patrick Brosset <:pbro>]

Attachment: Bug 1277834 - Silence inspector warnings and add promise rejection handlers;

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

Comment 13

[Patrick Brosset <:pbro>]

Attachment: Bug 1277834 - Add missing promise rejection handler to breadcrumbs;

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

Comment 14

[Patrick Brosset <:pbro>]

Honza: feel free to delegate the review if you've not worked with this code too much. But I figured that you've been doing a lot of inspector-related work lately.

The part 1 is about silencing warnings we've had for a long time in the inspector and that don't serve any purpose. They were logged when an error occurred after the panel was destroyed, which is expected. So now, we just bail out silently. This avoids creating confusing logs during tests.
It also adds a missing promise rejection to avoid huge stack traces in the logs too.

And part 2 is the actual fix for this bug: the test fails with an unhandled promise rejection, which I easily spotted in breadcrumbs.js.

Comment 15

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec7b6795cad7&group_state=expanded

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8772732 [details]
Bug 1277834 - Add missing promise rejection handler to breadcrumbs;

https://reviewboard.mozilla.org/r/65502/#review62974

I can still reproduce the test failure locally on Unbuntu, so I relaunched a bunch of jobs on your try run to see how it goes.

The failure is this.nodeHierarchy[this.currentIndex] is undefined. If the issue was related to the destroy of the breadcrumbs, I think the error should be "this.nodeHierarchy is null" (since this.nodeHierarchy is set to null in the destroy method of the breadcrumbs).

Let's wait for the outcome of the tests for now.

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8772731 [details]
Bug 1277834 - Silence inspector warnings and add promise rejection handlers;

https://reviewboard.mozilla.org/r/65500/#review62958

Looks good thanks!

I thought the idea was to wait on proper events in tests rather than swallowing errors in catch handlers?
I'm ok with this though, I think this increases the implementation's robustness and will decrease test complexity.

::: devtools/client/inspector/inspector-panel.js:267
(Diff revision 1)
> +      // Only log the rejection as an error if the panel hasn't been destroyed in the
> +      // meantime. We expect rejections when the panel is destroyed while requests are
> +      // still being processed.
> +      if (!this._panelDestroyer) {
> +        console.error(e);
> +      }

Around ~6 lines of trivial code duplicated 4 times in inspector-panel and 5 times in markup.js. This is most likely not what you want to read when you lookup those methods.

Maybe each class could have its own private method to cleanup this a bit.

  _catchPromiseError: function (e) {
    // Only log the rejection errors if the panel hasn't been destroyed in the meantime.
    if (!this._panelDestroyer) {
      console.error(e);
    }
  },

::: devtools/client/inspector/inspector-panel.js:634
(Diff revision 1)
>      // of the selected node so it can be restored after reload of the same page
>      if (this.canGetUniqueSelector &&
>          this.selection.isElementNode()) {
>        selection.getUniqueSelector().then(selector => {
>          this.selectionCssSelector = selector;
>        }).then(null, e => {

Not related to your changeset, but do you know if the pattern used here brings any value:

.then(success).then(null, failure)

instead of simply .then(success, failure) ?

Comment 18

[Julian Descottes [:jdescottes]]

Comment on attachment 8772732 [details]
Bug 1277834 - Add missing promise rejection handler to breadcrumbs;

https://reviewboard.mozilla.org/r/65502/#review63010

Ok, 0 failures on try on ~10 runs, so let's give this a shot. (https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec7b6795cad7&group_state=expanded)
Maybe my local tests hit an extremely rare intermittent that happened to have the same error message.
(I still don't understand the correlation between the fix and the error message, so I'll keep an eye on the test :) )

Comment 19

[Patrick Brosset <:pbro>]

(In reply to Julian Descottes [:jdescottes] from comment #16)
> The failure is this.nodeHierarchy[this.currentIndex] is undefined. If the
> issue was related to the destroy of the breadcrumbs, I think the error
> should be "this.nodeHierarchy is null" (since this.nodeHierarchy is set to
> null in the destroy method of the breadcrumbs).
Yeah, true. I'm not entirely sure what happens. But looking at the error message (unhandled promise rejection), it could really only come from one place: the waitForTick promise in the update method.
We call the scroll method in the handler, and that tries to access this.nodeHierarchy[this.currentIndex].
So my assumption was that during the test, the current inspector selection changed before the promise resolved, and so the this.nodeHierarchy array didn't have the same number of items anymore.

Thanks for retriggering jobs on linux!

(In reply to Julian Descottes [:jdescottes] from comment #17)
> 
> ::: devtools/client/inspector/inspector-panel.js:267
> (Diff revision 1)
> > +      // Only log the rejection as an error if the panel hasn't been destroyed in the
> > +      // meantime. We expect rejections when the panel is destroyed while requests are
> > +      // still being processed.
> > +      if (!this._panelDestroyer) {
> > +        console.error(e);
> > +      }
> 
> Around ~6 lines of trivial code duplicated 4 times in inspector-panel and 5
> times in markup.js. This is most likely not what you want to read when you
> lookup those methods.
> 
> Maybe each class could have its own private method to cleanup this a bit.
> 
>   _catchPromiseError: function (e) {
>     // Only log the rejection errors if the panel hasn't been destroyed in
> the meantime.
>     if (!this._panelDestroyer) {
>       console.error(e);
>     }
>   },
Good point! I'll make that change.

> ::: devtools/client/inspector/inspector-panel.js:634
> (Diff revision 1)
> >      // of the selected node so it can be restored after reload of the same page
> >      if (this.canGetUniqueSelector &&
> >          this.selection.isElementNode()) {
> >        selection.getUniqueSelector().then(selector => {
> >          this.selectionCssSelector = selector;
> >        }).then(null, e => {
> 
> Not related to your changeset, but do you know if the pattern used here
> brings any value:
> 
> .then(success).then(null, failure)
> 
> instead of simply .then(success, failure) ?
Right, there's no difference at all in this case.
But there are other cases where it makes a difference:

function p1() {
  return Promise.resolve("p1 resolved");
}

function p2() {
  return Promise.reject("p2 rejected");
}

p1().then(() => p2(), e => console.error(e));

Here we're not catching the rejection from p2. We'd only catch the one from p1 if it rejected.
So this would work better:

p1().then(() => p2()).then(null, e => console.error(e));
or simply:
p1().then(() => p2()).catch(e => console.error(e));

Comment 20

[Patrick Brosset <:pbro>]

Comment on attachment 8772731 [details]
Bug 1277834 - Silence inspector warnings and add promise rejection handlers;

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

Comment 21

[Patrick Brosset <:pbro>]

Comment on attachment 8772732 [details]
Bug 1277834 - Add missing promise rejection handler to breadcrumbs;

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

Comment 22

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=483370cc7073&group_state=expanded

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91c303acd4a3
Silence inspector warnings and add promise rejection handlers; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/2ce848f8b804
Add missing promise rejection handler to breadcrumbs; r=jdescottes

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/91c303acd4a3
https://hg.mozilla.org/mozilla-central/rev/2ce848f8b804

Comment 25

[Intermittent Failures Robot]

45 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-central: 13
* mozilla-inbound: 11
* fx-team: 8
* autoland: 6
* mozilla-aurora: 3
* ash: 3
* try: 1

Platform breakdown:
* linux64: 15
* windows7-32-vm: 14
* linux32: 12
* osx-10-10: 2
* windowsxp: 1
* Win 6.3.9600 x86_64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1277834&startday=2016-07-18&endday=2016-07-24&tree=all

Comment 26

[Ryan VanderMeulen [:RyanVM]]

This affects 49 as well. Can we please get a rebased patch (at least part 1 doesn't apply cleanly) and an approval request?

Comment 27

[Julian Descottes [:jdescottes]]

Looking at the recent failures, the current patch does not fix the intermittent. 
I don't think it is worth rebasing and uplifting just yet.

Comment 28

[Intermittent Failures Robot]

24 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 7
* fx-team: 6
* mozilla-aurora: 4
* mozilla-central: 3
* autoland: 2
* try: 1
* ash: 1

Platform breakdown:
* linux64: 16
* windows7-32-vm: 5
* linux32: 2
* osx-10-10: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1277834&startday=2016-07-25&endday=2016-07-31&tree=all

Comment 29

[Intermittent Failures Robot]

46 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 25
* autoland: 11
* mozilla-central: 3
* mozilla-beta: 2
* fx-team: 2
* try: 1
* mozilla-aurora: 1
* ash: 1

Platform breakdown:
* windows7-32-vm: 25
* linux64: 12
* linux32: 7
* osx-10-10: 1
* android-4-3-armv7-api15: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1277834&startday=2016-08-01&endday=2016-08-07&tree=all

Comment 30

[Intermittent Failures Robot]

5 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-beta: 3
* mozilla-aurora: 2

Platform breakdown:
* windows7-32-vm: 3
* windows8-64: 1
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1277834&startday=2016-08-08&endday=2016-08-14&tree=all

Comment 31

[Patrick Brosset <:pbro>]

https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1277834&startday=2016-07-01&endday=2016-08-22&tree=all shows that since August 7th, this test doesn't fail much anymore.

So indeed, the fix that I had done (and that landed on July 22nd) didn't fix the issue, but something that landed around August 7th must have.

The only thing I see that landed around that date which could have had an impact is bug 1267365. Especially this change: http://hg.mozilla.org/integration/fx-team/diff/1646172ac6e7/devtools/client/inspector/breadcrumbs.js
which changed waitForTick to setTimeout.

Anyway, adjusting the priority to reflect the failure frequency.

Comment 32

[Intermittent Failures Robot]

5 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-aurora: 3
* mozilla-beta: 2

Platform breakdown:
* linux64: 2
* linux32: 2
* windows7-32-vm: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1277834&startday=2016-08-29&endday=2016-09-04&tree=all

Comment 33

[Intermittent Failures Robot]

6 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-release: 2
* mozilla-beta: 2
* mozilla-aurora: 2

Platform breakdown:
* windows7-32-vm: 2
* linux64: 2
* linux32: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1277834&startday=2016-09-05&endday=2016-09-11&tree=all

Comment 34

[Intermittent Failures Robot]

5 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-aurora: 4
* mozilla-release: 1

Platform breakdown:
* windows7-32-vm: 3
* linux32: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1277834&startday=2016-09-12&endday=2016-09-18&tree=all