Closed Bug 1195881 Opened 9 years ago Closed 9 years ago

Set userContextId on the docShell for new remote (e10s) tabs

Categories

(Firefox :: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
e10s - ---
firefox46 --- fixed

People

(Reporter: englehardt, Assigned: kmckinley)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 19 obsolete files)

2.38 KB, patch
kmckinley
: review+
Details | Diff | Splinter Review
1.42 KB, patch
kmckinley
: review+
Details | Diff | Splinter Review
When a user creates a New Tab with a non-default value for the userContextId (Bug 1179557), this value should be set during the docShell's creation. In this bug, we'll add the plumbing necessary for value to propagate from the UI to the originAttibutes dictionary in the child docShell for remote (e10s) tabs. Bug 1191460 will implement this for non-e10s tabs where nsFrameLoader has direct access to create the docShell. See Bug 1191418 for more information on userContextId.
Assignee: nobody → senglehardt
Status: NEW → ASSIGNED
Depends on: 1165466, 1191740, 1179557
Attached patch 1195881-e10s.patch (obsolete) — Splinter Review
WIP patch from Bug 1191460. Will require re-write after changes in Bug 1191740.
Depends on: 1191460
tracking-e10s: --- → -
Attached patch 1195881-e10s.patch (obsolete) — Splinter Review
WIP
Attachment #8649417 - Attachment is obsolete: true
Assignee: bugzilla → kmckinley
Attachment #8652096 - Attachment is obsolete: true
Attachment #8697262 - Flags: review?(tanvi)
Attachment #8697262 - Attachment is obsolete: true
Attachment #8697262 - Flags: review?(tanvi)
Attachment #8697645 - Flags: review?(tanvi)
Minimized the patch & split out the NeckoParent changes for separate review
Attachment #8697645 - Attachment is obsolete: true
Attachment #8697645 - Flags: review?(tanvi)
Attachment #8698129 - Flags: review?(tanvi)
Attachment #8698129 - Flags: review?(jonas)
Attachment #8697267 - Flags: review?(tanvi) → review+
Comment on attachment 8698129 [details] [diff] [review] Contextual Identity working under e10s +nsDocShell::GetUserContextId(uint32_t* aUserContextId) Looks like we removed this from the patch in https://bugzilla.mozilla.org/show_bug.cgi?id=1191460#c48. Why are we adding it back in?
Comment on attachment 8698128 [details] [diff] [review] NeckoParent needs to propagate the userContextId dolske is not a necko peer, so he may not be the best person to flag here. https://wiki.mozilla.org/Modules/Core#Necko diff --git a/netwerk/ipc/NeckoParent.cpp b/netwerk/ipc/NeckoParent.cpp aAttrs = DocShellOriginAttributes(appId, inBrowserElement); aAttrs.mSignedPkg = tabContext.OriginAttributesRef().mSignedPkg; + aAttrs.mUserContextId = tabContext.OriginAttributesRef().mUserContextId; return nullptr; Can DocShellOriginAttributes take signedPkg and userContextID as parameters? Why does it only take appid and inBrowser? http://mxr.mozilla.org/mozilla-central/source/caps/BasePrincipal.h#100 Needinfo'ing Jonas to find out.
Flags: needinfo?(jonas)
Attachment #8698129 - Flags: review?(tanvi) → review-
Attachment #8698128 - Flags: review?(dolske) → review?(jduell.mcbugs)
Remove unneeded GetUserContextId, move setUserContextId into IDL so it is accessible from nsIDocShell
Attachment #8698129 - Attachment is obsolete: true
Attachment #8698129 - Flags: review?(jonas)
Attachment #8698275 - Flags: review?(tanvi)
Attachment #8698275 - Flags: review?(tanvi) → review+
Attachment #8698128 - Attachment is obsolete: true
Attachment #8698128 - Flags: review?(jduell.mcbugs)
Attachment #8698676 - Flags: review?(jonas)
Attachment #8698275 - Flags: review?(jonas)
Attachment #8698676 - Attachment is obsolete: true
Attachment #8698676 - Flags: review?(jonas)
Attachment #8699261 - Flags: review?(jonas)
Rebased to current master
Attachment #8697267 - Attachment is obsolete: true
Flags: needinfo?(jonas)
Attachment #8704759 - Flags: review?(jonas)
Attachment #8704759 - Attachment is obsolete: true
Attachment #8704759 - Flags: review?(jonas)
Attachment #8704846 - Flags: review+
Attachment #8698275 - Attachment is obsolete: true
Attachment #8698275 - Flags: review?(jonas)
Attachment #8704848 - Flags: review?(jonas)
Comment on attachment 8699261 [details] [diff] [review] NeckoParent needs to propagate the userContextId Review of attachment 8699261 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/ipc/NeckoParent.cpp @@ +159,2 @@ > aAttrs.mSignedPkg = tabContext.OriginAttributesRef().mSignedPkg; > + aAttrs.mUserContextId = tabContext.OriginAttributesRef().mUserContextId; Can you make this aAttrs.mUserContextId = aSerialized.mOriginAttributes.mUserContextId; instead. Eventually we should simply do "aAttrs = aSerialize.mOriginAttributes". But I don't want to mess with the appid stuff for now since it's broken. But reading the values out of aSerialize will make it more clear that that's where we're heading. Can you also change mSignedPkg to be read from aSerialize while you're at it (since that's actually even required for correctness)
Attachment #8699261 - Flags: review?(jonas) → review+
Comment on attachment 8698275 [details] [diff] [review] Contextual Identity working under e10s Review of attachment 8698275 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/TabChild.cpp @@ +837,5 @@ > } > + > + OriginAttributes attrs = OriginAttributesRef(); > + docShell->SetIsSignedPackage(attrs.mSignedPkg); > + docShell->SetUserContextId(attrs.mUserContextId); I hate that we're still forwarding these attribute-by-attribute. We should just do docShell->SetOriginAttributes(OriginAttributesRef()) here. But I suspect fixing that in a separate bug is safer.
Attachment #8698275 - Attachment is obsolete: false
Comment on attachment 8704848 [details] [diff] [review] Contextual Identity working under e10s Review of attachment 8704848 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/nsIDocShell.idl @@ +42,5 @@ > interface nsITabParent; > > typedef unsigned long nsLoadFlags; > > +[scriptable, builtinclass, uuid(32286b78-d35e-4f1b-a766-73d06060c29d)] Why this change? ::: dom/ipc/TabChild.cpp @@ +837,5 @@ > } > + > + OriginAttributes attrs = OriginAttributesRef(); > + docShell->SetIsSignedPackage(attrs.mSignedPkg); > + docShell->SetUserContextId(attrs.mUserContextId); (on correct patch now) I hate that we're still forwarding these attribute-by-attribute. We should just do docShell->SetOriginAttributes(OriginAttributesRef()) here. But I suspect fixing that in a separate bug is safer.
Attachment #8704848 - Flags: review?(jonas) → review+
Attachment #8698275 - Attachment is obsolete: true
Attachment #8704846 - Attachment is obsolete: true
Attachment #8705263 - Flags: review+
Consolidated the previous two patches, carried over r+ from tanvi and sicking on both
Attachment #8699261 - Attachment is obsolete: true
Attachment #8705263 - Attachment is obsolete: true
Attachment #8705264 - Flags: review+
Keywords: checkin-needed
Attachment #8704848 - Attachment is obsolete: true
Attachment #8705263 - Attachment is obsolete: false
Attachment #8705263 [details] [diff] is needed to enable tests under e10s
Keywords: checkin-needed
Blocks: 1239420
Back out restore userContextId for session store. We still save the userContextId, but we don't restore it yet. We need this since having contextual identity in e10s is currently higher priority than contextual identity support for session restore.
Attachment #8705263 - Attachment is obsolete: true
Attachment #8705264 - Attachment is obsolete: true
Attachment #8707574 - Flags: review+
Attached patch patch (obsolete) — Splinter Review
This patch plus what I proposed for bug 1239420 make e10s + sessionRestore working. Kate, can you tell me why you removed the setUserContextId() in SessionStore.jsm?
Attachment #8708487 - Flags: review?(kmckinley)
(In reply to Andrea Marchesini (:baku) from comment #28) > Created attachment 8708487 [details] [diff] [review] > patch > > This patch plus what I proposed for bug 1239420 make e10s + sessionRestore > working. > > Kate, can you tell me why you removed the setUserContextId() in > SessionStore.jsm? In Kate's most recent patch, she was trying to revert part of your patch to bug https://bugzilla.mozilla.org/show_bug.cgi?id=1193854 in order to make i) containers work with e10s ii) but not work with session restore. Getting it to work with e10s was more important than getting session restore to work. I asked her instead to backout 1193854 and land her original patch (comment 23) that was backed out. But it looks like that is no longer necessary, since you fixed bug 1239420! Hence, Kate can push her patch from comment 23 to try to make sure everything is still green and reland her patch. Thanks!
(In reply to Tanvi Vyas - please needinfo [:tanvi] from comment #29) > But it looks like that is no longer necessary, since you fixed bug 1239420! Nevermind, it was just backed out. But perhaps baku will have it fixed again soon.
Comment on attachment 8708487 [details] [diff] [review] patch Review of attachment 8708487 [details] [diff] [review]: ----------------------------------------------------------------- Reverting the patch to previous, so this is not needed.
Attachment #8708487 - Flags: review?(kmckinley) → review-
Attached patch test works under e10s (obsolete) — Splinter Review
when we use promiseTabState, the setting of the state we want happens too late in the tab lifecycle, causing a crash. This works with the 1239420 patch applied.
Attachment #8708855 - Flags: review?(tanvi)
Attachment #8708855 - Flags: review?(amarchesini)
This patch reverts back to depending on 1193854 and 1239420 to enable contextual identity under e10s.
Attachment #8707574 - Attachment is obsolete: true
Attachment #8708487 - Attachment is obsolete: true
Attachment #8708856 - Flags: review?(tanvi)
Attachment #8708856 - Flags: review?(amarchesini)
Attachment #8708856 - Attachment is patch: true
Comment on attachment 8708855 [details] [diff] [review] test works under e10s Leaving this review to baku since he wrote the original test.
Attachment #8708855 - Flags: review?(amarchesini)
Comment on attachment 8708855 [details] [diff] [review] test works under e10s Leaving this review to baku since he wrote the original test.
Attachment #8708855 - Flags: review?(tanvi)
Attachment #8708855 - Flags: review?(amarchesini)
Comment on attachment 8708856 [details] [diff] [review] Contextual Identity working under e10s with 1239420 patch You can probably also carry over Jonas' r+, since the code here is code he has previously r+'ed.
Attachment #8708856 - Flags: review?(tanvi) → review+
Attachment #8708855 - Flags: review?(amarchesini) → review+
Attachment #8708856 - Flags: review?(amarchesini) → review+
Attachment #8708856 - Attachment is obsolete: true
Attachment #8709862 - Flags: review+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[bugday-20160323] Status: RESOLVED,FIXED -> VERIFIED Comments: STR: Not clear. Developer specific testing Component: Name Firefox Version 46.0b4 Build ID 20160322075646 Update Channel beta User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 OS Windows 7 SP1 x86_64 Expected Results: Developer specific testing. Actual Results: As expected
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: