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

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: englehardt, Assigned: kmckinley)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s-, firefox46 fixed)

Details

Attachments

(2 attachments, 19 obsolete attachments)

2.38 KB, patch
kmckinley
: review+
Details | Diff | Splinter Review
1.42 KB, patch
kmckinley
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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

4 years ago
Assignee: nobody → senglehardt
Blocks: 1191418
Status: NEW → ASSIGNED
Depends on: 1165466, 1191740, 1179557
(Reporter)

Comment 1

4 years ago
Created attachment 8649417 [details] [diff] [review]
1195881-e10s.patch

WIP patch from Bug 1191460. Will require re-write after changes in Bug 1191740.
(Reporter)

Updated

4 years ago
Depends on: 1191460

Updated

4 years ago
tracking-e10s: --- → -
(Reporter)

Comment 2

3 years ago
Created attachment 8652096 [details] [diff] [review]
1195881-e10s.patch

WIP
Attachment #8649417 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Assignee: bugzilla → kmckinley
(Assignee)

Comment 3

3 years ago
Created attachment 8697262 [details] [diff] [review]
Contextual Identity working under e10s
Attachment #8652096 - Attachment is obsolete: true
Attachment #8697262 - Flags: review?(tanvi)
(Assignee)

Comment 4

3 years ago
Created attachment 8697267 [details] [diff] [review]
Enable the contextual identity tests to run under e10s
Attachment #8697267 - Flags: review?(tanvi)
(Assignee)

Comment 5

3 years ago
Created attachment 8697645 [details] [diff] [review]
Contextual Identity working under e10s
Attachment #8697262 - Attachment is obsolete: true
Attachment #8697262 - Flags: review?(tanvi)
Attachment #8697645 - Flags: review?(tanvi)
(Assignee)

Comment 6

3 years ago
Created attachment 8698128 [details] [diff] [review]
NeckoParent needs to propagate the userContextId
Attachment #8698128 - Flags: review?(dolske)
(Assignee)

Comment 7

3 years ago
Created attachment 8698129 [details] [diff] [review]
Contextual Identity working under e10s

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

3 years ago
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)

Updated

3 years ago
Attachment #8698129 - Flags: review?(tanvi) → review-
(Assignee)

Updated

3 years ago
Attachment #8698128 - Flags: review?(dolske) → review?(jduell.mcbugs)
Created attachment 8698275 [details] [diff] [review]
Contextual Identity working under e10s

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

3 years ago
Attachment #8698275 - Flags: review?(tanvi) → review+
Created attachment 8698676 [details] [diff] [review]
NeckoParent should copy all values from the OriginAttributes
Attachment #8698128 - Attachment is obsolete: true
Attachment #8698128 - Flags: review?(jduell.mcbugs)
Attachment #8698676 - Flags: review?(jonas)
(Assignee)

Updated

3 years ago
Attachment #8698275 - Flags: review?(jonas)
Created attachment 8699261 [details] [diff] [review]
NeckoParent needs to propagate the userContextId
Attachment #8698676 - Attachment is obsolete: true
Attachment #8698676 - Flags: review?(jonas)
Attachment #8699261 - Flags: review?(jonas)
Created attachment 8704759 [details] [diff] [review]
Contextual Identity working under e10s

Rebased to current master
Attachment #8697267 - Attachment is obsolete: true
Flags: needinfo?(jonas)
Attachment #8704759 - Flags: review?(jonas)
Created attachment 8704846 [details] [diff] [review]
Enable the contextual identity tests to run under e10s
Attachment #8704759 - Attachment is obsolete: true
Attachment #8704759 - Flags: review?(jonas)
Attachment #8704846 - Flags: review+
Created attachment 8704848 [details] [diff] [review]
Contextual Identity working under e10s
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

3 years ago
Attachment #8698275 - Attachment is obsolete: true
Created attachment 8705263 [details] [diff] [review]
Enable the contextual identity tests to run under e10s
Attachment #8704846 - Attachment is obsolete: true
Attachment #8705263 - Flags: review+
Created attachment 8705264 [details] [diff] [review]
Contextual Identity working under e10s

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

3 years ago
Keywords: checkin-needed
Attachment #8704848 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8705263 - Attachment is obsolete: false
Attachment #8705263 [details] [diff] is needed to enable tests under e10s
Keywords: checkin-needed
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Blocks: 1239420
Created attachment 8707574 [details] [diff] [review]
Contextual Identity working under e10s

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+
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?
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-
Created attachment 8708855 [details] [diff] [review]
test works under e10s

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)
Created attachment 8708856 [details] [diff] [review]
Contextual Identity working under e10s with 1239420 patch

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)

Updated

3 years ago
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)

Updated

3 years ago
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+
Created attachment 8709862 [details] [diff] [review]
Contextual Identity working under e10s
Attachment #8708856 - Attachment is obsolete: true
Attachment #8709862 - Flags: review+
Created attachment 8709863 [details] [diff] [review]
Update browser_sessionStoreContainer.js to work on under e10s.
Attachment #8708855 - Attachment is obsolete: true
Attachment #8709863 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 41

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5567470c17b7
https://hg.mozilla.org/mozilla-central/rev/49909767e030
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46

Comment 42

3 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.