Closed Bug 1253163 Opened 9 years ago Closed 9 years ago

[e10s] Fix browser_tabDrop.js to run in e10s

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 48
Tracking Flags:
Tracking Status
e10s + ---
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Patch for check-in
6.26 KB, patch
jaws
: review+
ritu
: approval-mozilla-aurora+
Details | Diff | Splinter Review
No description provided.
Comment on attachment 8726096 [details] MozReview Request: Bug 1253163 - [e10s] Fix browser_tabDrop.js to run in e10s. r?mattn https://reviewboard.mozilla.org/r/37805/#review34345 I can look closer at the rest tomorrow. ::: browser/base/content/test/general/browser_tabDrop.js:38 (Diff revision 1) > + let tabOpenEvent = yield awaitTabOpen; > + // stop search-engine loads from hitting the network > + tabOpenEvent.target.linkedBrowser.stop(); After looking at waitForDocLoadAndStopIt, I wonder if this will be intermittent with e10s as you may end up calling `stop` before the content even starts its new load? Or perhaps you end up stopping the initial about:blank load? waitForDocLoadAndStopIt is doing the stop in the content process.
Attachment #8726096 - Flags: review?(MattN+bmo)
https://reviewboard.mozilla.org/r/37805/#review34345 > After looking at waitForDocLoadAndStopIt, I wonder if this will be intermittent with e10s as you may end up calling `stop` before the content even starts its new load? Or perhaps you end up stopping the initial about:blank load? waitForDocLoadAndStopIt is doing the stop in the content process. about:blank isn't loaded in new tabs that have specified URLs since bug 878747. This URL is specified via the following code path: http://hg.mozilla.org/mozilla-central/annotate/2b5237c178ea/browser/base/content/tabbrowser.xml#l5902 http://hg.mozilla.org/mozilla-central/annotate/2b5237c178ea/browser/base/content/tabbrowser.xml#l1470 http://hg.mozilla.org/mozilla-central/annotate/2b5237c178ea/browser/base/content/tabbrowser.xml#l1900 http://hg.mozilla.org/mozilla-central/annotate/2b5237c178ea/browser/base/content/tabbrowser.xml#l1784 waitForDocLoadAndStopIt requires us to know the browser that the page would be loaded in, but it is a new browser created via the drop event. Using waitForDocLoadAndStopIt after the TabOpen event is too late, as once we get the reference to the tab's linkedBrowser via the event object, the only state change that waitForDocLoadAndStopIt sees is the STATE_STOP. I've changed this test to now use a custom search engine.
Comment on attachment 8726096 [details] MozReview Request: Bug 1253163 - [e10s] Fix browser_tabDrop.js to run in e10s. r?mattn Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37805/diff/1-2/
Attachment #8726096 - Flags: review?(MattN+bmo)
Attachment #8726096 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8726096 [details] MozReview Request: Bug 1253163 - [e10s] Fix browser_tabDrop.js to run in e10s. r?mattn https://reviewboard.mozilla.org/r/37805/#review34595 ::: browser/base/content/test/general/browser_tabDrop.js:19 (Diff revision 2) > +add_task(function* cleanup() { > + while (gBrowser.tabs.length > 1) { > + yield BrowserTestUtils.removeTab(gBrowser.tabs[gBrowser.tabs.length - 1]); > + } > + Services.search.currentEngine = originalEngine; > + let engine = Services.search.getEngineByName("MozSearch"); > + Services.search.removeEngine(engine); > - }); > +}); Isn't registerCleanupFunction still safer than a cleanup task in the event of an uncaught exception? IIRC you can pass a generator function to registerCleanupFunction. ::: browser/base/content/test/general/browser_tabDrop.js:51 (Diff revision 2) > + if (valid) { > + let tabOpenEvent = yield awaitTabOpen; > + info("Got TabOpen event"); Don't you need to check `awaitTabOpen` is truthy before the `yield` to prevent accidentally passing when the test should fail? ok(awaitTabOpen, …).
https://reviewboard.mozilla.org/r/37805/#review34595 > Isn't registerCleanupFunction still safer than a cleanup task in the event of an uncaught exception? IIRC you can pass a generator function to registerCleanupFunction. Yeah, I guess that is better. I'll switch it. > Don't you need to check `awaitTabOpen` is truthy before the `yield` to prevent accidentally passing when the test should fail? > ok(awaitTabOpen, …). The awaitTabOpen promise is only created if `valid` is true, so we don't need to check awaitTabOpen before entering this block. I will change the if-condition to check `awaitTabOpen` though as it is likely easier to follow.
Attachment #8726096 - Attachment is obsolete: true
Attachment #8727262 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2b0282e83350
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment on attachment 8727262 [details] [diff] [review] Patch for check-in Approval Request Comment [Feature/regressing bug #]: e10s test fix [User impact if declined]: n/a [Describe test coverage new/current, TreeHerder]: automated test [Risks and why]: no risk, doesn't touch production code [String/UUID change made/needed]: none
Attachment #8727262 - Flags: approval-mozilla-aurora?
Comment on attachment 8727262 [details] [diff] [review] Patch for check-in Test-only fix.
Attachment #8727262 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: