1239420 - Enable Contextual Identity support for SessionRestore

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Kate McKinley [:kmckinley, :Kate]]

Contextual Identity support for session restore was disabled by bug 1195881 so that we can have contextual identity running under e10s. Enabling the SessionStore to restore the userContextId causes a crash under e10s. See https://treeherder.mozilla.org/logviewer.html#?job_id=19474894&repo=mozilla-inbound for the crash.

Comment 1

[Jed Davis [:jld] (on leave until 08-31)]

This part looks bad:

18:34:27     INFO -  NeckoParent::AllocPHttpChannelParent: FATAL error: App does not have permission: KILLING CHILD PROCESS
18:34:27     INFO -  [Parent 1956] WARNING: Error constructing actor PHttpChannelParent: file /builds/slave/m-in-l64-d-0000000000000000000/build/src/obj-firefox/ipc/ipdl/PNeckoParent.cpp, line 1171
18:34:27     INFO -  ###!!! [Parent][DispatchAsyncMessage] Error: (msgtype=0xA00005,name=PNecko::Msg_PHttpChannelConstructor) Value error: message was deserialized, but contained an illegal value

Comment 2

[Andrea Marchesini [:baku]]

Attachment: a.patch

Comment 3

[Pulsebot]

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

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for bustage in https://hg.mozilla.org/integration/mozilla-inbound/rev/64ae16c5a281

https://treeherder.mozilla.org/logviewer.html#?job_id=19914433&repo=mozilla-inbound

Comment 5

[Andrea Marchesini [:baku]]

Attachment: a.patch

Sorry smaug but if you can review this patch again...
The previous one was breaking a test because we were resetting the userContextId all the time we where adding a new docShell child.
In this patch I added a check and we don't update the userContextId in case the current one is != 0.

Comment 6

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

I don't understand the new patch. If the original patch causes issues, that feels like a bug in the 
code using the API.

Comment 7

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Comment on attachment 8708678 [details] [diff] [review]
a.patch

So I don't see why we want to break the consistency in the API.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: a.patch

Comment 9

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Comment on attachment 8709134 [details] [diff] [review]
a.patch

>+  if (aChild->ItemType() == mItemType) {
>+    childDocShell->SetUserContextId(mUserContextId);
>+  }
>+
>   /* Set the child's global history if the parent has one */
>   if (mUseGlobalHistory) {
>     childDocShell->SetUseGlobalHistory(true);
>   }
> 
>   if (aChild->ItemType() != mItemType) {
>     return NS_OK;
>   }
You could just move the new code under this 'if() {}' and then
call childDocShell->SetUserContextId(mUserContextId); without if-check

> nsDocShell::SetUserContextId(uint32_t aUserContextId)
> {
>+  // We don't overwrite the userContextId in case the current one is not the
>+  // generic one.
>+  if (mUserContextId != nsIScriptSecurityManager::DEFAULT_USER_CONTEXT_ID) {
>+    return NS_OK;
>+  }
Drop this if. No need for special casing what UCI type

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f33275036d3

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f33275036d3