Closed Bug 1300822 Opened 3 years ago Closed 3 years ago

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

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(firefox50 fixed, firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

Details

Attachments

(1 file, 1 obsolete file)

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.
Yikes, I think I've been bitten by this before as well... let's fix it for sure! :)
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
Attachment #8788528 - Flags: review?(jryans) → review-
Ok, I have a new patch using BrowserTestUtils.browserLoaded.
I also converted much more code to use that.
Attachment #8788528 - Attachment is obsolete: true
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 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.
Attachment #8790347 - Flags: review?(ttromey) → review+
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/272f009cfe07
Fix tab load event wait in devtools test helpers. r=tromey
https://hg.mozilla.org/mozilla-central/rev/272f009cfe07
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.