Closed
Bug 1254145
Opened 8 years ago
Closed 8 years ago
BrowserTestUtils.withNewTab should use BrowserTestUtils.removeTab instead of gBrowser.removeTab
Categories
(Testing :: Mochitest, defect)
Tracking
(firefox48 fixed)
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jaws, Assigned: jaws)
Details
Attachments
(1 file, 2 obsolete files)
1.76 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8727426 -
Flags: review?(gijskruitbosch+bugs)
Comment 2•8 years ago
|
||
Comment on attachment 8727426 [details] [diff] [review] Patch Review of attachment 8727426 [details] [diff] [review]: ----------------------------------------------------------------- Good catch. I've been meaning to go through a number of the intermittent failure bugs that were filed for e10s where we're left with an extra tab at the end of the test, because I suspected these were caused by a lack of yield BTU.removeTab(). Here's for hoping this patch takes care of at least some of those.
Attachment #8727426 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 4•8 years ago
|
||
Backed out for failing bc7 tests. Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/6420f1afce76 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=912825aa7ee1 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=23209455&repo=mozilla-inbound 01:08:11 INFO - 129 INFO TEST-PASS | browser/base/content/test/general/browser_audioTabIcon.js | Expected the correct soundplaying attribute on the new tab - 01:08:11 INFO - 130 INFO TEST-PASS | browser/base/content/test/general/browser_audioTabIcon.js | Tooltips should not be equal - 01:08:11 INFO - 131 INFO TEST-PASS | browser/base/content/test/general/browser_audioTabIcon.js | Correct tooltip expected - 01:08:11 INFO - 132 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_audioTabIcon.js | Uncaught exception - at resource://testing-common/BrowserTestUtils.jsm:644 - TypeError: can't convert null to object 01:08:11 INFO - Stack trace: 01:08:11 INFO - removeTab/<@resource://testing-common/BrowserTestUtils.jsm:644:11 01:08:11 INFO - removeTab@resource://testing-common/BrowserTestUtils.jsm:643:12 01:08:11 INFO - this.BrowserTestUtils.withNewTab<@resource://testing-common/BrowserTestUtils.jsm:67:11 01:08:11 INFO - test_browser_swapping@chrome://mochitests/content/browser/browser/base/content/test/general/browser_audioTabIcon.js:287:9 01:08:11 INFO - Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23 01:08:11 INFO - this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7 01:08:11 INFO - Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11 01:08:11 INFO - this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7 01:08:11 INFO - this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:714:7 01:08:11 INFO - test_on_browser@chrome://mochitests/content/browser/browser/base/content/test/general/browser_audioTabIcon.js:436:11 01:08:11 INFO - test_on_browser@chrome://mochitests/content/browser/browser/base/content/test/general/browser_audioTabIcon.js:436:11 01:08:11 INFO - test_page@chrome://mochitests/content/browser/browser/base/content/test/general/browser_audioTabIcon.js:455:9 01:08:11 INFO - test_page@chrome://mochitests/content/browser/browser/base/content/test/general/browser_audioTabIcon.js:455:9 01:08:11 INFO - Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23 01:08:11 INFO - this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7 01:08:11 INFO - Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11 01:08:11 INFO - this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7 01:08:11 INFO - this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:714:7 01:08:11 INFO - setTimeout handler*SimpleTest_setTimeoutShim@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:621:12 01:08:11 INFO - SpecialPowersAPI.prototype._setTimeout@chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:653:7 01:08:11 INFO - SpecialPowersAPI.prototype.pushPrefEnv@chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:1049:7 01:08:11 INFO - @chrome://mochitests/content/browser/browser/base/content/test/general/browser_audioTabIcon.js:447:5 01:08:11 INFO - Promise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:388:5 01:08:11 INFO - @chrome://mochitests/content/browser/browser/base/content/test/general/browser_audioTabIcon.js:446:9 01:08:11 INFO - Tester_execTest@chrome://mochikit/content/browser-test.js:789:9 01:08:11 INFO - Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:709:7 01:08:11 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:740:59 01:08:11 INFO - 133 INFO Leaving test bound test_page
Flags: needinfo?(jaws)
Comment 5•8 years ago
|
||
It seems like that test uses withNewTab and then moves the tab's browser away by swapping it and closing the other, or something - but that means we're now left with a dead tab. We could nullcheck tab.linkedBrowser in BTU.removeTab() but that felt pretty hacky, and like it could potentially hide problems in other edgecases. Maybe the right fix is specifically detecting the case where the tab has been detached and is already closed (which I guess is the case here)? Either that or making the audio test not use BTU.withNewTab()...
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8727426 -
Attachment is obsolete: true
Flags: needinfo?(jaws)
Attachment #8740099 -
Flags: review?(gijskruitbosch+bugs)
Comment 7•8 years ago
|
||
Comment on attachment 8740099 [details] [diff] [review] Patch v2 Review of attachment 8740099 [details] [diff] [review]: ----------------------------------------------------------------- I'm a little confused by the duplicate promise... can you clarify? ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm @@ +75,5 @@ > } > } > let tab = yield BrowserTestUtils.openNewForegroundTab(options.gBrowser, options.url); > + let originalWindow = tab.ownerDocument.defaultView; > + let awaitTabClosed = BrowserTestUtils.removeTab({dontRemove: true}); Did you mean to yield for this in the else branch? If not, do we need it?
Attachment #8740099 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 8•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=61589832bbc6
Assignee | ||
Comment 9•8 years ago
|
||
I had meant to remove that promise.
Attachment #8740099 -
Attachment is obsolete: true
Attachment #8740132 -
Flags: review?(gijskruitbosch+bugs)
Updated•8 years ago
|
Attachment #8740132 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/12e42798a150
Keywords: checkin-needed
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/12e42798a150
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•