1310276 - Assertion failure: docShellAttrs.mUserContextId == attrs.mUserContextId (docshell and necko should have the same userContextId attribute.)

Status: RESOLVED DUPLICATE of bug 1301649

(Core :: DOM: Security, defect, P1)

Description

[Kamil Jozwiak [:kjozwiak]]

Attachment: stack.txt

I'm running into the following crash when opening about:preferences in the default container while another website is also being loaded in the personal container at the same time. I can consistently reproduce under macOS 10.12 using xCode 8.0 (8A218a).

STR:

* launch the latest version m-c debug
* open a personal container via File -> New Container Tab
* open a regular tab via File -> New Tab
* load gmail.com within the personal container
* while gmail.com is loading, load about:preference in the regular tab (default container)

Assertion failure: docShellAttrs.mUserContextId == attrs.mUserContextId (docshell and necko should have the same userContextId attribute.), at /Users/kjozwiak/projects/m-c/netwerk/protocol/http/HttpBaseChannel.cpp:2948
#01: mozilla::net::HttpBaseChannel::SetupReplacementChannel(nsIURI*, nsIChannel*, bool, unsigned int)[/Users/kjozwiak/projects/m-c/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x877c24]
#02: mozilla::net::nsHttpChannel::SetupReplacementChannel(nsIURI*, nsIChannel*, bool, unsigned int)[/Users/kjozwiak/projects/m-c/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x8ed6f9]
#03: mozilla::net::nsHttpChannel::ContinueProcessRedirectionAfterFallback(nsresult)[/Users/kjozwiak/projects/m-c/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x8f7050]
#04: mozilla::net::nsHttpChannel::AsyncProcessRedirection(unsigned int)[/Users/kjozwiak/projects/m-c/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x8df584]
#05: mozilla::net::nsHttpChannel::ContinueProcessResponse1(nsresult)[/Users/kjozwiak/projects/m-c/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x8e882d]
#06: mozilla::net::nsHttpChannel::ProcessResponse()[/Users/kjozwiak/projects/m-c/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x8e83f7]
#07: mozilla::net::nsHttpChannel::OnStartRequest(nsIRequest*, nsISupports*)[/Users/kjozwiak/projects/m-c/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x8ff174]
#08: non-virtual thunk to mozilla::net::nsHttpChannel::OnStartRequest(nsIRequest*, nsISupports*)[/Users/kjozwiak/projects/m-c/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x8ff837]
#09: nsInputStreamPump::OnStateStart()[/Users/kjozwiak/projects/m-c/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x35f395]
#10: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*)[/Users/kjozwiak/projects/m-c/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x35eebd]
#11: non-virtual thunk to nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*)[/Users/kjozwiak/projects/m-c/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x36002c]
#12: nsInputStreamReadyEvent::Run()[/Users/kjozwiak/projects/m-c/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x190329]
#13: nsThread::ProcessNextEvent(bool, bool*)[/Users/kjozwiak/projects/m-c/objdir-ff-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1cdc19]

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #0)
> 
> * launch the latest version m-c debug
> * open a personal container via File -> New Container Tab
> * load gmail.com within the personal container
I think an easier STR is just open gmail in a container tab.

And this happens in fetching favicon.
The favicon request will be redirected from https://accounts.google.com/favicon.ico to https://www.google.com/favicon.ico.
I guess this happens after bug 1277803 landed last week.
The favicon is loaded by XUL in chrome process, however it uses OA from the content side.

But Kamil's first report is 10 hours earlier than bug 1277803 landed in m-c.
https://bugzilla.mozilla.org/show_bug.cgi?id=1308938#c9
https://bugzilla.mozilla.org/show_bug.cgi?id=1277803#c119

Kamil, in your first report (https://bugzilla.mozilla.org/show_bug.cgi?id=1308938#c9), have bug 1277803 already landed in m-c?

Or this is another problem ?

Thanks

Comment 2

[Kamil Jozwiak [:kjozwiak]]

Attachment: linuxStack.txt

(In reply to Yoshi Huang[:allstars.chh] from comment #1)
> Kamil, in your first report
> (https://bugzilla.mozilla.org/show_bug.cgi?id=1308938#c9), have bug 1277803
> already landed in m-c?
> 
> Or this is another problem ?
> 
> Thanks

I think this might be a separate issue. I pulled the latest m-c changes and I can still reproduce the same issue when loading gmail.com inside a container while using a debug build. I've also managed to reproduce the crash under Ubuntu as well.

Yoshi, is there anything else that I can do to help narrow this down?

STR:

* load gmail.com in a container while using a debug version of m-c

Platforms Used:

* macOS 10.12 x64 - Reproduced
** fx52.0a1, buildId: 20161018213306, changeset: 01ab78dd9880+ tip
* Ubuntu 16.04.1 x64 - Reproduced
** fx52.0a1, buildId: 20161018211930, changeset: 01ab78dd9880 tip

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #2)
> Yoshi, is there anything else that I can do to help narrow this down?
I think I understand the root cause now.
This happens even before bug 1277803, actually I think this happens after bug 1301649 is landed.
In Bug 1260931 Part 4 patch, I added assertion for top level document loads, but in bug 1301649 the assertion will be tested for every kind of load, not just loading top level document. And in the favicon loading, it will have problems.

For favicon loading, it will have two loads, one from Places library, and the other from xul:image.
For Places library loading, it doesn't have loadContext,
and for xul:image cases, the loadContext will be chrome docshell since the load is done by chrome process.
So it doesn't make sense to compare the origin attributes between loadInfo and loadContext if it's for favicon loading. I'll skip the check accordingly.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

I am still working on the test.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8802440 [details] [diff] [review]
Patch.

Review of attachment 8802440 [details] [diff] [review]:
-----------------------------------------------------------------

Hi smaug
I am about to have a PTO soon, not sure if I have enough time to have a test for this.
Perhaps the test will be provided by other team member in a seperate patch or even a seperate bug.

I already have some brief explanation in comment 3.
So the assert is triggered in when Places library loading favicon,
at this time the loadContext is null, and the loadInfo is from the content principal with content origin attributes, that's why the mismatch is triggered.

Also for the xul:image case we should NOT do the check either,
because the loadContext will be chrome docshell, and loadInfo now has the origin attributes from content side. (done in bug 1277803)

So I skip the check for favicon entirely.

Thanks

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8802440 [details] [diff] [review]
Patch.

>+    bool isFavicon =
>+      newLoadInfo->InternalContentPolicyType() == nsIContentPolicy::TYPE_INTERNAL_IMAGE_FAVICON;
>+    if (!isFavicon) {
Rather odd to special case favicon this way. 


>+      nsCOMPtr<nsILoadContext> loadContext;
>+      NS_QueryNotificationCallbacks(this, loadContext);
>+      DocShellOriginAttributes docShellAttrs;
>       loadContext->GetOriginAttributes(docShellAttrs);
This looks crash-prone. Using loadContext without null check. Please don't remove the null check.

>-    }
> 
>-    // top-level docshell shouldn't have firstPartyDomain attribute.
>-    MOZ_ASSERT_IF(isTopLevelDoc, docShellAttrs.mFirstPartyDomain.IsEmpty());
>+      // re-compute the origin attributes of the loadInfo if it's top-level load.
>+      bool isTopLevelDoc =
>+        newLoadInfo->GetExternalContentPolicyType() == nsIContentPolicy::TYPE_DOCUMENT;
>+
>+      // top-level docshell shouldn't have firstPartyDomain attribute.
>+      MOZ_ASSERT_IF(isTopLevelDoc, docShellAttrs.mFirstPartyDomain.IsEmpty());
> 
>-    NeckoOriginAttributes attrs = newLoadInfo->GetOriginAttributes();
>+      NeckoOriginAttributes attrs = newLoadInfo->GetOriginAttributes();
> 
>-    MOZ_ASSERT(docShellAttrs.mAppId == attrs.mAppId,
>-               "docshell and necko should have the same appId attribute.");
>-    MOZ_ASSERT(docShellAttrs.mUserContextId == attrs.mUserContextId,
>-               "docshell and necko should have the same userContextId attribute.");
>-    MOZ_ASSERT(docShellAttrs.mInIsolatedMozBrowser == attrs.mInIsolatedMozBrowser,
>-               "docshell and necko should have the same inIsolatedMozBrowser attribute.");
>-    MOZ_ASSERT(docShellAttrs.mPrivateBrowsingId == attrs.mPrivateBrowsingId,
>-               "docshell and necko should have the same privateBrowsingId attribute.");
>+      MOZ_ASSERT(docShellAttrs.mAppId == attrs.mAppId,
>+                 "docshell and necko should have the same appId attribute.");
>+      MOZ_ASSERT(docShellAttrs.mUserContextId == attrs.mUserContextId,
>+                 "docshell and necko should have the same userContextId attribute.");
>+      MOZ_ASSERT(docShellAttrs.mInIsolatedMozBrowser == attrs.mInIsolatedMozBrowser,
>+                 "docshell and necko should have the same inIsolatedMozBrowser attribute.");
>+      MOZ_ASSERT(docShellAttrs.mPrivateBrowsingId == attrs.mPrivateBrowsingId,
>+                 "docshell and necko should have the same privateBrowsingId attribute.");
> 
>-    attrs.InheritFromDocShellToNecko(docShellAttrs, isTopLevelDoc, newURI);
>-    newLoadInfo->SetOriginAttributes(attrs);
>+      attrs.InheritFromDocShellToNecko(docShellAttrs, isTopLevelDoc, newURI);
>+      newLoadInfo->SetOriginAttributes(attrs);
>+    }
So, chrome initiated loads with some random OA will get OA overridden with system principal OA. Is that what we want?
Would it make sense to check if docshell is chrome docshell and not inherit?
Or is there particular reason you special case favicon loading? Could you explain.

But at least add that null check back.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

I might not finish this before the end of today, this is caused by a minor bug bug 1301649, so I plan to back out bug 1301649, and I'll continue to address smaug's comments there.

Comment 8

[Kamil Jozwiak [:kjozwiak]]

Thanks Yoshi, I'm not seeing this anymore after bug#1301649comment#9 was backed out. Used the STR from comment#2 using the following platforms/builds:

* macOS 10.12 x64 - PASSED
** fx52.0a1, buildId: 20161020103958, changeset: b04e95606673+ tip

* Ubuntu 16.04.1 LTS x64 VM - PASSED
** fx52.0a1, buildId: 20161020110202, changeset: 3f0aeafe59c4 tip