Closed Bug 1254145 Opened 4 years ago Closed 4 years ago

BrowserTestUtils.withNewTab should use BrowserTestUtils.removeTab instead of gBrowser.removeTab

Categories

(Testing :: Mochitest, defect)

Version 3
defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jaws, Assigned: jaws)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
Attachment #8727426 - Flags: review?(gijskruitbosch+bugs)
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+
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)
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()...
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #8727426 - Attachment is obsolete: true
Flags: needinfo?(jaws)
Attachment #8740099 - Flags: review?(gijskruitbosch+bugs)
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)
Attached patch Patch v3Splinter Review
I had meant to remove that promise.
Attachment #8740099 - Attachment is obsolete: true
Attachment #8740132 - Flags: review?(gijskruitbosch+bugs)
Attachment #8740132 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/12e42798a150
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.