Clean up other tabs participating in the GroupedSHistory when removing a tab

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
5 months ago

People

(Reporter: Nika, Assigned: Nika)

Tracking

(Blocks 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

This is a first step toward 1310770 - namely it handles deleting the other tabs participating in the same GroupedSHistory when closing the tab. It doesn't handle dragging between windows yet unfortunately, though I have plans for what that may look like.
MozReview-Commit-ID: KiMsKOljNpE
Attachment #8811938 - Flags: review?(ehsan)
Attachment #8811938 - Flags: feedback?(sawang)
Comment on attachment 8811938 [details] [diff] [review]
Remove inactive hidden tabs when removing the primary tab

Review of attachment 8811938 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_close_dependent_tabs.js
@@ +28,5 @@
> +  });
> +
> +  // At this point tab2 should be closed
> +  todo(!tab2.linkedBrowser, "The new tab should be closed");
> +  yield BrowserTestUtils.removeTab(tab2); // XXX: Shouldn't be needed once the todo is fixed

I'm assuming you'll be fixing this soon?

::: dom/base/GroupedSHistory.cpp
@@ +173,5 @@
> +{
> +  for (int32_t i = 0; i < mPartialHistories.Length(); ++i) {
> +    if (i != mIndexOfActivePartialHistory) {
> +      nsCOMPtr<nsIFrameLoader> loader;
> +      mPartialHistories[i]->GetOwnerFrameLoader(getter_AddRefs(loader));

Other code here null checks mPartialHistories[i]....

Also is loader guaranteed to be non-null?
Attachment #8811938 - Flags: review?(ehsan) → review+
Comment on attachment 8811938 [details] [diff] [review]
Remove inactive hidden tabs when removing the primary tab

Looks good.

I assume our plan for navigating to chrome process url such as about:config is using updateBrowserRemoteness, and closeInactiveFrameLoaderOwners can also be used in this case, which is nice.

(In reply to :Ehsan Akhgari from comment #2)
> Comment on attachment 8811938 [details] [diff] [review]
> ::: dom/base/GroupedSHistory.cpp
> @@ +173,5 @@
> > +{
> > +  for (int32_t i = 0; i < mPartialHistories.Length(); ++i) {
> > +    if (i != mIndexOfActivePartialHistory) {
> > +      nsCOMPtr<nsIFrameLoader> loader;
> > +      mPartialHistories[i]->GetOwnerFrameLoader(getter_AddRefs(loader));
> 
> Other code here null checks mPartialHistories[i]....
> 
> Also is loader guaranteed to be non-null?

May all be unnecessary. It was the nervous first time cycle collector user of me adding null checks everywhere...
Attachment #8811938 - Flags: feedback?(sawang) → feedback+
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4669804cc49
Remove inactive hidden tabs when removing the primary tab, r=ehsan
Backed out for failing browser_close_dependent_tabs.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2e450275deefe6855206ae14493a89dbe68977d

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c4669804cc496d815bc7438a19d3ef23459abc2f
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=39814337&repo=mozilla-inbound

[task 2016-11-24T18:11:33.570831Z] 18:11:33     INFO - TEST-PASS | browser/base/content/test/general/browser_close_dependent_tabs.js | The browser changed process! - 
[task 2016-11-24T18:11:33.571020Z] 18:11:33     INFO - Buffered messages finished
[task 2016-11-24T18:11:33.572984Z] 18:11:33     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_close_dependent_tabs.js | Uncaught exception - [Exception... "[JavaScript Error: "Unknown userContextId!" {file: "resource://gre/components/UnifiedComplete.js" line: 370}]'[JavaScript Error: "Unknown userContextId!" {file: "resource://gre/components/UnifiedComplete.js" line: 370}]' when calling method: [mozIPlacesAutoComplete::unregisterOpenPage]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://browser/content/tabbrowser.xml :: _beginRemoveTab :: line 2627"  data: yes]
[task 2016-11-24T18:11:33.577387Z] 18:11:33     INFO - Stack trace:
[task 2016-11-24T18:11:33.578731Z] 18:11:33     INFO -     _beginRemoveTab@chrome://browser/content/tabbrowser.xml:2627:15
[task 2016-11-24T18:11:33.580402Z] 18:11:33     INFO -     removeTab@chrome://browser/content/tabbrowser.xml:2480:18
[task 2016-11-24T18:11:33.581863Z] 18:11:33     INFO -     removeTab/<@resource://testing-common/BrowserTestUtils.jsm:823:9
[task 2016-11-24T18:11:33.583412Z] 18:11:33     INFO -     removeTab@resource://testing-common/BrowserTestUtils.jsm:813:12
[task 2016-11-24T18:11:33.588354Z] 18:11:33     INFO -     this.BrowserTestUtils.withNewTab<@resource://testing-common/BrowserTestUtils.jsm:82:13
[task 2016-11-24T18:11:33.590102Z] 18:11:33     INFO -     @chrome://mochitests/content/browser/browser/base/content/test/general/browser_close_dependent_tabs.js:35:9
[task 2016-11-24T18:11:33.592436Z] 18:11:33     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:737:9
[task 2016-11-24T18:11:33.594453Z] 18:11:33     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:657:7
[task 2016-11-24T18:11:33.599372Z] 18:11:33     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:744:59
[task 2016-11-24T18:11:33.601171Z] 18:11:33     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:737:9
[task 2016-11-24T18:11:33.602773Z] 18:11:33     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:657:7
[task 2016-11-24T18:11:33.604449Z] 18:11:33     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:744:59
[task 2016-11-24T18:11:33.606131Z] 18:11:33     INFO - Leaving test bound
Flags: needinfo?(michael)
This needs to be landed at the same time as bug 1318767 and that failure should go away.
Flags: needinfo?(michael)
try server now builds with -Werror=sign-compare so it needs update.
Attachment #8811938 - Attachment is obsolete: true
Attachment #8818214 - Attachment is obsolete: true
Pushed by echen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dbec5fef561
Remove inactive hidden tabs when removing the primary tab, r=ehsan
https://hg.mozilla.org/mozilla-central/rev/5dbec5fef561
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.