Bug 1125916 has a work around for a loadInfo-loadContext mismatch in channels created by imgLoader. This bug should find a real fix.

This is like Bug 1266022, I assume moz-icon is only used by XUL(?), so ignore the check for loadinfo.

Comment on attachment 8775507 [details] [diff] [review] Part 1: inherit OA from docshell. Hi Jonas This should be the mistake I made in Bug 1263496 (Part 3). I found in those tests under dom/browser-element, the origin attributes of those data:text/html tests are inherited from the owner, (for example, owner will be http://mochitest:8888/.../dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html) but they should be inherited from the docshell of current iframe(mozbrowser frame) So I've fixed to inherit from docshell, Could you review this for me ? Thanks

Comment on attachment 8775509 [details] [diff] [review] Part 2: ignore moz-icon when checking loadInfo. Review of attachment 8775509 [details] [diff] [review]: ----------------------------------------------------------------- Should we also check for blob URIs? blob:null-uuid

Hi Jonas There is one remaing problem I am still not sure, what's the right way to fix loading image that uses a SystemPrincipal? For example, the test mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1125916#c89 test_focus_blur_manage_events.html (currently it's disabled now), the loadingPrincipal is SystemPrincipal, and same case for the about:newtab to load a thumbnail image Can you provide some suggestions? Thanks

I tried to override the origin attributes of the loadInfo when doing image load.

Comment on attachment 8776529 [details] [diff] [review] Part 4: override loadInfo's origin attributes in imgLoader Review of attachment 8776529 [details] [diff] [review]: ----------------------------------------------------------------- Why is this needed? Isn't the purpose of this bug exactly to remove this?

Comment on attachment 8776529 [details] [diff] [review] Part 4: override loadInfo's origin attributes in imgLoader yeah, probably a bad idea to move it back.

Hi Jonas I still have questions regarding to loading image. Take the test ./dom/inputmethod/mochitest/test_focus_blur_manage_events.html for instance as you mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1125916#c89 In this test it will create a mozbrowser frame to load another html http://searchfox.org/mozilla-central/rev/3df383b8552c1f8059f5c21258388ddb5a2f33d0/dom/inputmethod/mochitest/test_focus_blur_manage_events.html#170 At this time, nsDocShell::DoURILoad will try to load the test file, also set the origin attributes on the loadInfo. http://searchfox.org/mozilla-central/rev/3df383b8552c1f8059f5c21258388ddb5a2f33d0/docshell/base/nsDocShell.cpp#10841 since the docshell now is for mozbrowser, so it will have the isIsolatedMozBrowser flag is true. And since the path of the test file is chrome://mochitests/content/chrome/dom/inputmethod/mochitest/file_test_focus_blur_manage_events.html, so the owner of the channel will be the System Principal. http://searchfox.org/mozilla-central/rev/3df383b8552c1f8059f5c21258388ddb5a2f33d0/chrome/nsChromeProtocolHandler.cpp#194 And this principal(or owner) will in turn become the NodePrincipal of this document, which in turn will become the loadingPrincipal of the following sub-resources requests. So the problem I saw is, for the channel to load the test file(file_test_focus_blur_manage_events.html), its loadInfo has isIsolatedMozBrowser flag set, however its owner is System Principal, which doesn't have the mozbrowser flag, and we use that SystemPrincipal to load sub-resources, it looks that we didn't propagate the origin attributes to the sub-resource loading (for example, for the top-level document load, we have a loadInfo with correct origin attributes, but not for those loadInfos of sub-resource loading), Should the loadInfos of sub-resources 'inherit' the origin attributes? Or should I ignore the check of calling NS_CompareLoadInfoAndLoadContext when the loadingPrincipal is SystemPrincipal? could you kindly help to share some comments on this? :D Thanks

I don't understand comment 12. First off, the searchfox link appears to be wrong since it's pointing at error handling code and not something that's related to OA or loadInfo. Second, I'm not sure what "owner" you are referring to. Are you talking about the "owner" in docshell? Docshell generally loads html/xul pages, but the assertion we are hitting is when imglib is loading an image. Are you saying that the testcase is loading an image into a docshell, rather than loading a HTML/XUL page?

I think the test_focus_blur_manage_events.html test doesn't actually want isolation. The best way to fix the assertion there is likely to fix bug 1254823. The other situation is about:newtab. about:newtab runs with system privileges, so the document there has a system principal, but runs in a docshell which might have a userContextId set. For reasons that I think are off-topic to this bug, about:newtab is not possible to fix without making bigger changes. So what I think we should do in this bug is to avoid the assertion when the loadingDocument is a about:newtab page.

Hi Jonas, This patch prevent checking loadInfo for about:newtab, I still keep the moz-icon, for it's needed by about:sync-tabs, and it's not related to mozbrowser so I think it doesn't related to bug 1254823. If you still have time, could you review for me? Thanks

Comment on attachment 8777284 [details] [diff] [review] Part 4: skip checking for about:newtab Review of attachment 8777284 [details] [diff] [review]: ----------------------------------------------------------------- Hi Tanvi It looks Jonas can't review this. :( Could you review this? So this part fixed Jonas' comment 14, avoid the asserion when the loadingDocument is about:newtab. However I found in nsBaseChannel.cpp there's no documentURI, so I use scheme.EqualsLiteral("blob") && isSystemPrinicipal to check this. (the blob url is used to load some icons in about:newtab) Thanks

Comment on attachment 8777284 [details] [diff] [review] Part 4: skip checking for about:newtab Review of attachment 8777284 [details] [diff] [review]: ----------------------------------------------------------------- Jonas said he could finish this. :D

Comment on attachment 8777284 [details] [diff] [review] Part 4: skip checking for about:newtab Review of attachment 8777284 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsBaseChannel.cpp @@ +655,5 @@ > mURI->GetScheme(scheme); > + bool isSystem = nsContentUtils::IsSystemPrincipal(mLoadInfo->LoadingPrincipal()); > + if (!(scheme.EqualsLiteral("file") || > + scheme.EqualsLiteral("moz-icon") || > + (scheme.EqualsLiteral("blob") && isSystem))) { I don't think we should skip blob. Why is that needed? Rather than excluding moz-icon and blob, I think we should do the same as in httpchannel and look at the document uri, which you can get from mLoadInfo->LoadingNode()->OwnerDoc()->GetDocumentURI() (or some such, with nullchecks of course). Then exclude abotu:newtab and about:sync-tabs

Comment on attachment 8777284 [details] [diff] [review] Part 4: skip checking for about:newtab Review of attachment 8777284 [details] [diff] [review]: ----------------------------------------------------------------- Another thing that would be a good idea is to *always* assert that for http requests, that mPrivateBrowsingId in the two OAs match. That way we know that we're not affecting private browsing. We should assert that no matter if the request is coming from about:newtab or about:sync-tabs or if it's a system principal which does the loading.

merged part 2 and part 4 together

Comment on attachment 8777675 [details] [diff] [review] Part 2: skip checking for about:newtab and about:sync-tabs Review of attachment 8777675 [details] [diff] [review]: ----------------------------------------------------------------- It would probably be cleaner to move this into NS_CompareLoadInfoAndLoadContext, so it doesn't have to be duplicated in two places. But given that these assertions are likely going away soon, I think this is ok.

Comment on attachment 8777676 [details] [diff] [review] Part 4: compare privateBrowsingId for HTTP channel Review of attachment 8777676 [details] [diff] [review]: ----------------------------------------------------------------- Since there's just one caller of this function. I'd say just make the function static and local to nsHttpChannel.cpp. No need to treat this as a function that is part of the necko API. Or even inline at the callsite. Up to you. r=me with that fixed.

moved those checks inside NS_CompareLoadInfoAndLoadContext.

check in the call site

Hi Kanru See the commit message for the detail. Thanks

The part 4 patch (compare mPrivateBrowsingId between LoadInfo and LoadContext) will trigger asserion on try https://treeherder.mozilla.org/#/jobs?repo=try&revision=7480f005031c&selectedJob=25092619 I am checking this.

Hi jdm This should be some leftovers in Bug 1269231, could you review this for me? Thanks

Comment on attachment 8778064 [details] [diff] [review] Part 6: sync privateBrowsing in LoadInfo. wait, I still found some test failed.

I think PrivateBrowsing has a bigger problem, so I'll file another bug to fix PB. The problem I saw that is the origin attributes of the loadInfo and LoadContext are already wrong, they don't have the mPrivateBrowsingId set(mPrivateBrowsingId is 0), however the loadInfo->GetUsePrivateBrowsing() and loadContext->GetUserPrivateBrowsing() are both true. And somehow after IPC, I guess somewhere calls attrs.SyncAttributesWithPrivateBrowsing to update the loadContext's origin attributes, so we'll have an assertion on the parent side.

Pushed by yhuang@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c2dbbb2abaef Part 1: inherit OA from docshell. r=sicking https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a4bb4e923c Part 2: skip checking for about:newtab and about:sync-tabs r=sicking https://hg.mozilla.org/integration/mozilla-inbound/rev/a6b262acd1b3 Part 3: remove loadInfo work-around. r=sicking https://hg.mozilla.org/integration/mozilla-inbound/rev/46d487ebf9af Part 4: add a TODO in test. r=kanru

https://hg.mozilla.org/mozilla-central/rev/c2dbbb2abaef https://hg.mozilla.org/mozilla-central/rev/b1a4bb4e923c https://hg.mozilla.org/mozilla-central/rev/a6b262acd1b3 https://hg.mozilla.org/mozilla-central/rev/46d487ebf9af