Closed Bug 1239420 Opened 4 years ago Closed 4 years ago

Enable Contextual Identity support for SessionRestore

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: kmckinley, Assigned: baku)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
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
Assignee: nobody → amarchesini
Attached patch a.patch (obsolete) — Splinter Review
Attachment #8708485 - Flags: review?(bugs)
Attachment #8708485 - Flags: review?(bugs) → review+
Attached patch a.patch (obsolete) — Splinter Review
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.
Attachment #8708485 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Attachment #8708678 - Flags: review?(bugs)
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 on attachment 8708678 [details] [diff] [review]
a.patch

So I don't see why we want to break the consistency in the API.
Attachment #8708678 - Flags: review?(bugs) → review-
Attached patch a.patchSplinter Review
Attachment #8708678 - Attachment is obsolete: true
Attachment #8709134 - Flags: review?(bugs)
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
Attachment #8709134 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/2f33275036d3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.