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)
Firefox
Security
Tracking
()
RESOLVED
FIXED
Firefox 46
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.
Reporter | ||
Updated•9 years ago
|
Reporter | ||
Comment 1•9 years ago
|
||
WIP patch from Bug 1191460. Will require re-write after changes in Bug 1191740.
Updated•9 years ago
|
tracking-e10s:
--- → -
Assignee | ||
Updated•9 years ago
|
Assignee: bugzilla → kmckinley
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8652096 -
Attachment is obsolete: true
Attachment #8697262 -
Flags: review?(tanvi)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8697267 -
Flags: review?(tanvi)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8697262 -
Attachment is obsolete: true
Attachment #8697262 -
Flags: review?(tanvi)
Attachment #8697645 -
Flags: review?(tanvi)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8698128 -
Flags: review?(dolske)
Assignee | ||
Comment 7•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8697267 -
Flags: review?(tanvi) → review+
Comment 8•9 years ago
|
||
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 9•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8698129 -
Flags: review?(tanvi) → review-
Assignee | ||
Updated•9 years ago
|
Attachment #8698128 -
Flags: review?(dolske) → review?(jduell.mcbugs)
Assignee | ||
Comment 10•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8698275 -
Flags: review?(tanvi) → review+
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8698128 -
Attachment is obsolete: true
Attachment #8698128 -
Flags: review?(jduell.mcbugs)
Attachment #8698676 -
Flags: review?(jonas)
Assignee | ||
Comment 12•9 years ago
|
||
Assignee | ||
Comment 13•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8698275 -
Flags: review?(jonas)
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8698676 -
Attachment is obsolete: true
Attachment #8698676 -
Flags: review?(jonas)
Attachment #8699261 -
Flags: review?(jonas)
Assignee | ||
Comment 15•9 years ago
|
||
Rebased to current master
Attachment #8697267 -
Attachment is obsolete: true
Flags: needinfo?(jonas)
Attachment #8704759 -
Flags: review?(jonas)
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8704759 -
Attachment is obsolete: true
Attachment #8704759 -
Flags: review?(jonas)
Attachment #8704846 -
Flags: review+
Assignee | ||
Comment 17•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Attachment #8698275 -
Attachment is obsolete: true
Assignee | ||
Comment 21•9 years ago
|
||
Attachment #8704846 -
Attachment is obsolete: true
Attachment #8705263 -
Flags: review+
Assignee | ||
Comment 22•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Updated•9 years ago
|
Attachment #8704848 -
Attachment is obsolete: true
Comment 23•9 years ago
|
||
Keywords: checkin-needed
Assignee | ||
Updated•9 years ago
|
Attachment #8705263 -
Attachment is obsolete: false
Assignee | ||
Comment 24•9 years ago
|
||
Attachment #8705263 [details] [diff] is needed to enable tests under e10s
Keywords: checkin-needed
Comment 25•9 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/312ccc646df3 for e10s failures in browser_sessionStoreContainer.js like https://treeherder.mozilla.org/logviewer.html#?job_id=19474894&repo=mozilla-inbound
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 26•9 years ago
|
||
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+
Assignee | ||
Comment 27•9 years ago
|
||
Comment 28•9 years ago
|
||
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)
Comment 29•9 years ago
|
||
(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!
Comment 30•9 years ago
|
||
(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.
Assignee | ||
Comment 31•9 years ago
|
||
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-
Assignee | ||
Comment 32•9 years ago
|
||
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)
Assignee | ||
Comment 33•9 years ago
|
||
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)
Assignee | ||
Comment 34•9 years ago
|
||
Treeherder push with 1239420 patch applied.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e17f2100daf6
Updated•9 years ago
|
Attachment #8708856 -
Attachment is patch: true
Comment 35•9 years ago
|
||
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 36•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8708855 -
Flags: review?(amarchesini)
Comment 37•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8708855 -
Flags: review?(amarchesini) → review+
Updated•9 years ago
|
Attachment #8708856 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 38•9 years ago
|
||
Attachment #8708856 -
Attachment is obsolete: true
Attachment #8709862 -
Flags: review+
Assignee | ||
Comment 39•9 years ago
|
||
Attachment #8708855 -
Attachment is obsolete: true
Attachment #8709863 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 40•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/5567470c17b7
https://hg.mozilla.org/integration/fx-team/rev/49909767e030
Keywords: checkin-needed
Comment 41•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5567470c17b7
https://hg.mozilla.org/mozilla-central/rev/49909767e030
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Comment 42•9 years ago
|
||
[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.
Description
•