1300822 - addTab and refreshTab from shared-head.js possibly resolve sooner than expected

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Waiting for "load" event on a <xul:browser> from a privileged code can be misleading as you also receive "load" event for iframes loading in the top document loaded in the frame.
So that addTab helper which is used almost everywhere can sometime resolve sooner than expected anytime a document has iframes.

I saw that exact scenario happen in gcli helper listenOnce used in browser_cmd_csscoverage_startstop.js:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/commandline/test/browser_cmd_csscoverage_startstop.js?q=file%3Abrowser_cmd_csscoverage_startstop.js&redirect_type=single#49
  // Page 2 is a frame in page 1. JS in the page navigates to page 3.
  yield helpers.listenOnce(options.browser, "load", true);
This line was always resolving on the "Page 2 frame from page 1" load event.
And as addTab use the exact same approach, I'm expecting it to have the same issue.

Comment 1

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Attachment: Bug 1300822 - Fix tab load event wait in devtools test helpers.

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

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Yikes, I think I've been bitten by this before as well... let's fix it for sure! :)

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8788528 [details]
Bug 1300822 - Fix tab load event wait in devtools test helpers.

https://reviewboard.mozilla.org/r/76992/#review75126

It looks like `BrowserTestUtils.browserLoaded`[1] handles the top-frame-only case already.  Can we use this helper so that we stay in sync with browser team?  (BrowserTestUtils is auto-loaded by the mochitest browser harness.)

[1]: https://dxr.mozilla.org/mozilla-central/rev/8c9c4e816e86f903c1d820f3f29715dc070a5a4a/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#183

Comment 4

[Alexandre Poirot [:ochameau] [PTO back August 21]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=652d69c1c600

Comment 5

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Ok, I have a new patch using BrowserTestUtils.browserLoaded.
I also converted much more code to use that.

Comment 6

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Attachment: Bug 1300822 - Fix tab load event wait in devtools test helpers.

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

Comment 7

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Tom, Ryan is on PTO for quite a while, so I'm lurking for alternative reviewers.
Hopefully you won't mind this r?. Sorry about this depressive patch. It is quite significant but I'm only converting things to stop listening for "load" event manually on <xul:browser> element.
Instead it uses BrowserTestUtils.browserLoaded which ensure ignoring the load event for inner iframes.

It should kill intermittents in any test where we didn't knew about that.

Comment 8

[Tom Tromey :tromey]

Comment on attachment 8790347 [details]
Bug 1300822 - Fix tab load event wait in devtools test helpers.

https://reviewboard.mozilla.org/r/78228/#review76958

Thank you.
I read through this and everything looked good.
This is a great cleanup and fix.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/272f009cfe07
Fix tab load event wait in devtools test helpers. r=tromey

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/272f009cfe07

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0437e13b60c3