Closed Bug 1260766 Opened 4 years ago Closed 3 years ago

Moving tabs with non-default user context between windows needs rework

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox48 --- affected
firefox49 --- fixed

People

(Reporter: jryans, Assigned: baku)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

As part of bug 1242644 (about to land), swapFrameLoaders will start to enforce that both frames being swapped have matching OriginAttributes before allow them to be swapped.

This breaks moving tabs between windows (which uses swapFrameLoaders) for tabs with non-default user context IDs.  (Regular tabs using the default context continue to move as expected.)

Most likely the issue is that the user context ID needs to be set earlier on the target browser we swap into when moving tabs.
Duplicate of this bug: 1267538
Assignee: nobody → amarchesini
Attached patch fix.patch (obsolete) — Splinter Review
Attachment #8745853 - Flags: review?(bugs)
Comment on attachment 8745853 [details] [diff] [review]
fix.patch

Please add a comment that this expects that usercontextid attribute has been set on the xul:browser before swap. Don't know how to explain that clearly in a comment though.

+    int32_t namespaceID = mOwnerContent->GetNameSpaceID();
+    if ((namespaceID == kNameSpaceID_XUL) &&
I would use just mOwnerContent->IsXULElement()
Attachment #8745853 - Flags: review?(bugs) → review+
Attached patch fix.patch (obsolete) — Splinter Review
Attachment #8745853 - Attachment is obsolete: true
Attachment #8745874 - Flags: review?(bugs)
Comment on attachment 8745874 [details] [diff] [review]
fix.patch

You need to re-explain to me, why this works if "our" isn't the about:blank, but the actual page which we want to move to a new window.
Attached patch fix.patchSplinter Review
Attachment #8745874 - Attachment is obsolete: true
Attachment #8745874 - Flags: review?(bugs)
Attachment #8745985 - Flags: review?(bugs)
Comment on attachment 8745985 [details] [diff] [review]
fix.patch

You need to use the other frameloader for PopulateUserContextIdFromAttribute call when populating otherOriginAttributes. In both places.
With that r+
Attachment #8745985 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/e78a24e7938b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.