Currently the loadInfo for TYPE_DOCUMENT loads does not hold the requesting context which caused Bug 1329288. Please note that for all other subresource loads the loadInfo holds the correct context. For TYPE_DOCUMENT loads we probably want to store to actual window and not only the window id as we do right now.

Please note that this bug should also remove the ContentPolicy check from within docshell and solely rely on the ConPol checks from the ContentSecurityManager. When doing so, please also remove the special case handling within WebRequestContent.js (shoulLoad) which we added within Bug 1329288.

Thanks for the heads-up. How is TB affected by this, which interfaces change?

Smaug, given our focus on performance, we should try to get that bug fixed within the FF57 timeframe. At the moment we are performing two (more or less identical) content policy checks for loads of TYPE_DOCUMENT as well as TYPE_SUBDOCUMENT. We already tried to remove the contentpolicy check within Bug 1182569, but then had to bring it back within Bug 1329288 because of the missing accurate context for toplevel content policy checks. We have to be really careful to not introduce any regressions again. We only have a few fully automated tests (see [1,2,3]), so I guess we also have to manually verify given the information from Bug 1329288. A first TRY run (see comment 2) seems promising, but I am running into some problems when running [1] locally: 1) the context.name is the empty string. I can't recall why I used context.name instead of following your suggestion from [4], but I guess that needs to be updated. Any idea why context.name might be empty now but not in the old world? 2) The following assetion within js/src/vm/Interpreter.cpp fires. > MOZ_ASSERT(!cx->enableAccessValidation || > cx->compartment()->isAccessValid()); When commenting out that assertion, the test runs to completion. Any suggestions on how to debug that or what might be wrong here? Finally, just to confirm, are we on the right track for fixing this bug or am I missing something completely? [1] docshell/test/navigation/test_contentpolicy_block_window.html [2] toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html [3] dom/security/test/hsts/ [4] https://bugzilla.mozilla.org/show_bug.cgi?id=1329288#c12

Comment on attachment 8900192 [details] [diff] [review] bug_1331740_contentpolicy_docshell.patch Jorg, this time I am letting you know ahead of time that I am about to change that behavior. Can you also please keep an eye on this bug so we are not regressing anything? Thanks!

(In reply to Jorg K (GMT+2) from comment #3) > Thanks for the heads-up. How is TB affected by this, which interfaces change? Oh, I missed you already replied :-) I guess it broke a bunch of TB stuff, e.g. TB Mozmill test suite [see 1]. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1329288#c18

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5) > Can you also please keep an eye on this bug so we are not regressing anything? I'm subscribed now :-)

Comment on attachment 8900192 [details] [diff] [review] bug_1331740_contentpolicy_docshell.patch I don't know about the js/src/vm/Interpreter.cpp issue. Do you know which of those two checks fails? What happens if we don't still have mScriptGlobal?

(In reply to Olli Pettay [:smaug] from comment #8) > Comment on attachment 8900192 [details] [diff] [review] > bug_1331740_contentpolicy_docshell.patch > > I don't know about the js/src/vm/Interpreter.cpp issue. > Do you know which of those two checks fails? Since this assertion is firing: > MOZ_ASSERT(!cx->enableAccessValidation || > cx->compartment()->isAccessValid()); I think we are trying to run script which global is no longer alive. I am not entirely sure, but the comment here [1] indicates this is the case. > What happens if we don't still have mScriptGlobal? Probably this is answered by the question above. Is there anything we can do about that? [1] https://dxr.mozilla.org/mozilla-central/source/js/src/jsfriendapi.h#3059-3064

Hmm, if mScriptGlobal is not alive anymore, then docshell shouldn't load anything. But there is also the case that mScriptGlobal might not yet be there. Do we need to call EnsureScriptEnvironment() ? I still wonder if we want to pass the window object. In non-e10s we pass the chrome xul:browser. Should we pass the TabChildGlobal in e10s case?

(In reply to Olli Pettay [:smaug] from comment #10) > Hmm, if mScriptGlobal is not alive anymore, then docshell shouldn't load > anything. > But there is also the case that mScriptGlobal might not yet be there. After chatting on IRC I think I realized what you meant in comment 8. If mScriptGlobal is null, like in the initial case when loading a page (and setting up docshell) then the context for the contentPolicy check should be null. This is also the case in the current world when we perform the content policy check within docshell. > Do we need to call EnsureScriptEnvironment() ? Given my previous comment, I don't think we should perform anything special there. > I still wonder if we want to pass the window object. In non-e10s we pass the > chrome xul:browser. > Should we pass the TabChildGlobal in e10s case? For this bug I would like to pass (if possible) the exactly same arguments to the content policy check within the contentSecurityManager than we pass to the contentPolicy check within docshell at the moment. In other words, I think we should just try to somehow store that information within the loadInfo so we can pass it to contentPolicy checks before opening the channel.

Bill, Steve, I saw you added the following assertion [1] within js/src/vm/Interpreter.cpp: MOZ_ASSERT(!cx->enableAccessValidation || cx->compartment()->isAccessValid()); Within this bug we would like to remove the contentPolicy check from within docshell and only consult content policy checks within the contentSecurityManager. In other words, we are not performing the content policy check within docshell itself, but rather shortly before we open the channel, which happens at a later point than setting up docshell for the new load. Anyway, the attached patch roughly shows what we are changing. Now, when running test_contentpolicy_block_window.html the above assertion fires. At this point I am not entirely sure what that means. Is it possible, since we are loading docshell, that some variables already ran out of scope and hence the access is not valid anymore? Thanks for any insights! [1] https://hg.mozilla.org/mozilla-central/annotate/4caca1d0ba0e35cbe57a88493ebf162aa2cb3144/js/src/vm/Interpreter.cpp#l370

Calling this [qf:p1] since it seems like it'll speed up pageload a bit - but please let us know if this doesn't seem doable for 57, and we can retriage as [qf:p2] (post-57 high-priority work).

I don't think we will be able to speed up page load (based on some discussions on IRC earlier today etc). But feel free to put qf back, if I'm wrong.

Can you post a stack for the assertion failure, Christoph? The assertion here is meant to catch bugs in Quantum DOM labeling. I would guess that the problem here is that we labeled a runnable as SystemGroup, and now we're running content policy code in some mochitest document. This probably isn't a problem outside of tests since content policies aren't supposed to run content code. We'll still need to fix this somehow, though.

(In reply to Olli Pettay [:smaug] from comment #14) > I don't think we will be able to speed up page load (based on some > discussions on IRC earlier today etc). > But feel free to put qf back, if I'm wrong. I feel different, at the moment we perform 2 (more or less identical) content policy checks for docshell loads. I agree that for Firefox itself this might be a rather minor optimization, but would still slightly improve performance. When you think of any kind of content blocker that can hook into nsIContentPolicy however, then this might be a different story and performing 2 content policy checks instead of one might speed up things quite a bit.

(In reply to Bill McCloskey (:billm) from comment #15) > Can you post a stack for the assertion failure, Christoph? The assertion > here is meant to catch bugs in Quantum DOM labeling. I would guess that the > problem here is that we labeled a runnable as SystemGroup, and now we're > running content policy code in some mochitest document. This probably isn't > a problem outside of tests since content policies aren't supposed to run > content code. We'll still need to fix this somehow, though. #0 0x00007fffe6ce1e59 in js::RunScript (cx=0x7fffd9205000, state=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:379 #1 0x00007fffe6ce2659 in js::InternalCallOrConstruct (cx=0x7fffd9205000, args=..., construct=js::NO_CONSTRUCT) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:496 #2 0x00007fffe6ce28f4 in InternalCall (cx=0x7fffd9205000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:523 #3 0x00007fffe6ce29ae in js::Call (cx=0x7fffd9205000, fval=..., thisv=..., args=..., rval=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:542 #4 0x00007fffe731ec27 in js::fun_apply (cx=0x7fffd9205000, argc=2, vp=0x7fffffff54d0) at /home/ckerschb/source/mc/js/src/jsfun.cpp:1306 #5 0x00007fffe6d0771c in js::CallJSNative (cx=0x7fffd9205000, native=0x7fffe731e704 <js::fun_apply(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/ckerschb/source/mc/js/src/jscntxtinlines.h:293 #6 0x00007fffe6ce257b in js::InternalCallOrConstruct (cx=0x7fffd9205000, args=..., construct=js::NO_CONSTRUCT) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:478 #7 0x00007fffe6ce28f4 in InternalCall (cx=0x7fffd9205000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:523 #8 0x00007fffe6ce29ae in js::Call (cx=0x7fffd9205000, fval=..., thisv=..., args=..., rval=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:542 #9 0x00007fffe744ec29 in js::ForwardingProxyHandler::call (this=0x7fffed271990 <xpc::XrayWaiver>, cx=0x7fffd9205000, proxy=..., args=...) at /home/ckerschb/source/mc/js/src/proxy/Wrapper.cpp:175 #10 0x00007fffe74449df in js::Proxy::call (cx=0x7fffd9205000, proxy=..., args=...) at /home/ckerschb/source/mc/js/src/proxy/Proxy.cpp:497 #11 0x00007fffe7445b00 in js::proxy_Call (cx=0x7fffd9205000, argc=2, vp=0x7fffffff5910) at /home/ckerschb/source/mc/js/src/proxy/Proxy.cpp:757 #12 0x00007fffe6d0771c in js::CallJSNative (cx=0x7fffd9205000, native=0x7fffe7445a19 <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/ckerschb/source/mc/js/src/jscntxtinlines.h:293 #13 0x00007fffe6ce23ad in js::InternalCallOrConstruct (cx=0x7fffd9205000, args=..., construct=js::NO_CONSTRUCT) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:460 #14 0x00007fffe6ce28f4 in InternalCall (cx=0x7fffd9205000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:523 #15 0x00007fffe6ce29ae in js::Call (cx=0x7fffd9205000, fval=..., thisv=..., args=..., rval=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:542 #16 0x00007fffe744ec29 in js::ForwardingProxyHandler::call (this=0x7fffed2719b0 <xpc::WaiveXrayWrapper::singleton>, cx=0x7fffd9205000, proxy=..., args=...) at /home/ckerschb/source/mc/js/src/proxy/Wrapper.cpp:175 #17 0x00007fffe7433005 in js::CrossCompartmentWrapper::call (this=0x7fffed2719b0 <xpc::WaiveXrayWrapper::singleton>, cx=0x7fffd9205000, wrapper=..., args=...) at /home/ckerschb/source/mc/js/src/proxy/CrossCompartmentWrapper.cpp:359 #18 0x00007fffe11cf5dc in xpc::WaiveXrayWrapper::call (this=0x7fffed2719b0 <xpc::WaiveXrayWrapper::singleton>, cx=0x7fffd9205000, wrapper=..., args=...) at /home/ckerschb/source/mc/js/xpconnect/wrappers/WaiveXrayWrapper.cpp:72 #19 0x00007fffe74449df in js::Proxy::call (cx=0x7fffd9205000, proxy=..., args=...) at /home/ckerschb/source/mc/js/src/proxy/Proxy.cpp:497 #20 0x00007fffe7445b00 in js::proxy_Call (cx=0x7fffd9205000, argc=2, vp=0x7fffd6241350) at /home/ckerschb/source/mc/js/src/proxy/Proxy.cpp:757 #21 0x00007fffe6d0771c in js::CallJSNative (cx=0x7fffd9205000, native=0x7fffe7445a19 <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/ckerschb/source/mc/js/src/jscntxtinlines.h:293 #22 0x00007fffe6ce23ad in js::InternalCallOrConstruct (cx=0x7fffd9205000, args=..., construct=js::NO_CONSTRUCT) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:460 #23 0x00007fffe6ce28f4 in InternalCall (cx=0x7fffd9205000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:523 #24 0x00007fffe6ce2932 in js::CallFromStack (cx=0x7fffd9205000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:529 #25 0x00007fffe6cf0a6d in Interpret (cx=0x7fffd9205000, state=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:3074 #26 0x00007fffe6ce20e9 in js::RunScript (cx=0x7fffd9205000, state=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:418 #27 0x00007fffe6ce2659 in js::InternalCallOrConstruct (cx=0x7fffd9205000, args=..., construct=js::NO_CONSTRUCT) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:496 #28 0x00007fffe6ce28f4 in InternalCall (cx=0x7fffd9205000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:523 #29 0x00007fffe6ce29ae in js::Call (cx=0x7fffd9205000, fval=..., thisv=..., args=..., rval=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:542 #30 0x00007fffe72ad66c in JS_CallFunctionValue (cx=0x7fffd9205000, obj=..., fval=..., args=..., rval=...) at /home/ckerschb/source/mc/js/src/jsapi.cpp:2852 #31 0x00007fffe1282443 in nsXPCWrappedJSClass::CallMethod (this=0x7fffc1f6f830, wrapper=0x7fffc1f6be80, methodIndex=3, info_=0x7fffd62e0330, nativeParams=0x7fffffff6e80) at /home/ckerschb/source/mc/js/xpconnect/src/XPCWrappedJSClass.cpp:1318 #32 0x00007fffe127c461 in nsXPCWrappedJS::CallMethod (this=0x7fffc1f6be80, methodIndex=3, info=0x7fffd62e0330, params=0x7fffffff6e80) at /home/ckerschb/source/mc/js/xpconnect/src/XPCWrappedJS.cpp:615 #33 0x00007fffe019484f in PrepareAndDispatch (self=0x7fffc1f7ebe0, methodIndex=3, args=0x7fffffff6fc0, gpregs=0x7fffffff6f40, fpregs=0x7fffffff6f70) at /home/ckerschb/source/mc/xpcom/reflect/xptcall/md/unix/xptcstubs_x86_64_linux.cpp:120 #34 0x00007fffe019490f in SharedStub () at /home/ckerschb/source/mc/xpcom/reflect/xptcall/md/unix/xptcstubs_x86_64_linux.cpp:126 #35 0x00007fffe1e4a8e6 in nsContentPolicy::CheckPolicy (this=0x7fffd4d79ab0, policyMethod=&virtual nsIContentPolicy::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString const&, nsISupports*, nsIPrincipal*, short*), contentType=6, contentLocation=0x7fffc1fe3af0, requestingLocation=0x0, requestingContext=0x0, mimeType=<gNullChar> "", extra=0x0, requestPrincipal=0x7fffdd2b0120, decision=0x7fffffff7350) at /home/ckerschb/source/mc/dom/base/nsContentPolicy.cpp:156 #36 0x00007fffe1dd62d8 in nsContentPolicy::ShouldLoad (this=0x7fffd4d79ab0, contentType=6, contentLocation=0x7fffc1fe3af0, requestingLocation=0x0, requestingContext=0x0, mimeType=<gNullChar> "", extra=0x0, requestPrincipal=0x7fffdd2b0120, decision=0x7fffffff7350) at /home/ckerschb/source/mc/dom/base/nsContentPolicy.cpp:218 #37 0x00007fffe1b02f8e in NS_CheckContentLoadPolicy (contentType=6, contentLocation=0x7fffc1fe3af0, originPrincipal=0x7fffdd2b0120, context=0x0, mimeType=<gNullChar> "", ---Type <return> to continue, or q <return> to quit--- extra=0x0, decision=0x7fffffff7350, policyService=0x7fffd4d79ab0) at /home/ckerschb/source/mc/dom/base/nsContentPolicyUtils.h:228 #38 0x00007fffe37c5c51 in DoContentSecurityChecks (aChannel=0x7fffd4ff1a10, aLoadInfo=0x7fffc1f68050) at /home/ckerschb/source/mc/dom/security/nsContentSecurityManager.cpp:415 #39 0x00007fffe37c60cd in nsContentSecurityManager::doContentSecurityCheck (aChannel=0x7fffd4ff1a10, aInAndOutListener=[(nsDocumentOpenInfo *) 0x7fffc1da7f40]) at /home/ckerschb/source/mc/dom/security/nsContentSecurityManager.cpp:502 #40 0x00007fffe02628a2 in nsBaseChannel::AsyncOpen2 (this=0x7fffd4ff19c0, aListener=0x7fffc1da7f40) at /home/ckerschb/source/mc/netwerk/base/nsBaseChannel.cpp:725 #41 0x00007fffe13d8ea4 in nsURILoader::OpenURI (this=0x7fffc493b940, channel=0x7fffd4ff1a10, aFlags=0, aWindowContext=0x7fffdb01a030) at /home/ckerschb/source/mc/uriloader/base/nsURILoader.cpp:843 #42 0x00007fffe63b264e in nsDocShell::DoChannelLoad (this=0x7fffdb01a000, aChannel=0x7fffd4ff1a10, aURILoader=0x7fffc493b940, aBypassClassifier=false) at /home/ckerschb/source/mc/docshell/base/nsDocShell.cpp:11645 #43 0x00007fffe63b1c90 in nsDocShell::DoURILoad (this=0x7fffdb01a000, aURI=0x7fffc1fe3af0, aOriginalURI=0x0, aResultPrincipalURI=..., aLoadReplace=false, aLoadFromExternal=false, aReferrerURI=0x0, aSendReferrer=true, aReferrerPolicy=0, aTriggeringPrincipal=0x7fffdd2b0120, aPrincipalToInherit=0x0, aTypeHint=0x0, aFileName=<gNullChar> u"", aPostData=0x0, aHeadersData=0x0, aFirstParty=true, aDocShell=0x0, aRequest=0x7fffffff7b60, aIsNewWindowTarget=false, aBypassClassifier=false, aForceAllowCookies=false, aSrcdoc=<gNullChar> u"", aBaseURI=0x0, aContentPolicyType=6) at /home/ckerschb/source/mc/docshell/base/nsDocShell.cpp:11450 #44 0x00007fffe63ae666 in nsDocShell::InternalLoad (this=0x7fffdb01a000, aURI=0x7fffc1fe3af0, aOriginalURI=0x0, aResultPrincipalURI=..., aLoadReplace=false, aReferrer=0x0, aReferrerPolicy=0, aTriggeringPrincipal=0x7fffdd2b0120, aPrincipalToInherit=0x0, aFlags=1, aWindowTarget=u"", aTypeHint=0x0, aFileName=<gNullChar> u"", aPostData=0x0, aHeadersData=0x0, aLoadType=1, aSHEntry=0x0, aFirstParty=true, aSrcdoc=<gNullChar> u"", aSourceDocShell=0x0, aBaseURI=0x0, aCheckForPrerender=false, aDocShell=0x0, aRequest=0x0) at /home/ckerschb/source/mc/docshell/base/nsDocShell.cpp:10744 #45 0x00007fffe638a109 in nsDocShell::LoadURI (this=0x7fffdb01a000, aURI=0x7fffc1fe3af0, aLoadInfo=0x7fffc1d64a20, aLoadFlags=0, aFirstParty=true) at /home/ckerschb/source/mc/docshell/base/nsDocShell.cpp:1620 #46 0x00007fffe6396a0d in nsDocShell::LoadURIWithOptions (this=0x7fffdb01a000, aURI=0x7fffc1dbb500 u"about:newtab", aLoadFlags=0, aReferringURI=0x0, aReferrerPolicy=0, aPostStream=0x0, aHeaderStream=0x0, aBaseURI=0x0, aTriggeringPrincipal=0x0) at /home/ckerschb/source/mc/docshell/base/nsDocShell.cpp:4929 #47 0x00007fffe0193d96 in NS_InvokeByIndex () at /home/ckerschb/source/mc/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:106 #48 0x00007fffe12a583d in CallMethodHelper::Invoke (this=0x7fffffff8390) at /home/ckerschb/source/mc/js/xpconnect/src/XPCWrappedNative.cpp:1996 #49 0x00007fffe12a32d3 in CallMethodHelper::Call (this=0x7fffffff8390) at /home/ckerschb/source/mc/js/xpconnect/src/XPCWrappedNative.cpp:1315 #50 0x00007fffe12881cf in XPCWrappedNative::CallMethod (ccx=..., mode=XPCWrappedNative::CALL_METHOD) at /home/ckerschb/source/mc/js/xpconnect/src/XPCWrappedNative.cpp:1282 #51 0x00007fffe129108f in XPC_WN_CallMethod (cx=0x7fffd9205000, argc=8, vp=0x7fffd6241280) at /home/ckerschb/source/mc/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:927 #52 0x00007fffe6d0771c in js::CallJSNative (cx=0x7fffd9205000, native=0x7fffe1290d2e <XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/ckerschb/source/mc/js/src/jscntxtinlines.h:293 #53 0x00007fffe6ce257b in js::InternalCallOrConstruct (cx=0x7fffd9205000, args=..., construct=js::NO_CONSTRUCT) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:478 #54 0x00007fffe6ce28f4 in InternalCall (cx=0x7fffd9205000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:523 #55 0x00007fffe6ce2932 in js::CallFromStack (cx=0x7fffd9205000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:529 #56 0x00007fffe6cf0a6d in Interpret (cx=0x7fffd9205000, state=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:3074 #57 0x00007fffe6ce20e9 in js::RunScript (cx=0x7fffd9205000, state=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:418 #58 0x00007fffe6ce2659 in js::InternalCallOrConstruct (cx=0x7fffd9205000, args=..., construct=js::NO_CONSTRUCT) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:496 #59 0x00007fffe6ce28f4 in InternalCall (cx=0x7fffd9205000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:523 #60 0x00007fffe6ce2932 in js::CallFromStack (cx=0x7fffd9205000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:529 #61 0x00007fffe6cf0a6d in Interpret (cx=0x7fffd9205000, state=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:3074 #62 0x00007fffe6ce20e9 in js::RunScript (cx=0x7fffd9205000, state=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:418 #63 0x00007fffe6ce2659 in js::InternalCallOrConstruct (cx=0x7fffd9205000, args=..., construct=js::NO_CONSTRUCT) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:496 #64 0x00007fffe6ce28f4 in InternalCall (cx=0x7fffd9205000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:523 #65 0x00007fffe6ce2932 in js::CallFromStack (cx=0x7fffd9205000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:529 #66 0x00007fffe6cf0a6d in Interpret (cx=0x7fffd9205000, state=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:3074 #67 0x00007fffe6ce20e9 in js::RunScript (cx=0x7fffd9205000, state=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:418 #68 0x00007fffe6ce2659 in js::InternalCallOrConstruct (cx=0x7fffd9205000, args=..., construct=js::NO_CONSTRUCT) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:496 #69 0x00007fffe6ce28f4 in InternalCall (cx=0x7fffd9205000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:523 #70 0x00007fffe6ce2932 in js::CallFromStack (cx=0x7fffd9205000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:529 #71 0x00007fffe6cf0a6d in Interpret (cx=0x7fffd9205000, state=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:3074 #72 0x00007fffe6ce20e9 in js::RunScript (cx=0x7fffd9205000, state=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:418 #73 0x00007fffe6ce2659 in js::InternalCallOrConstruct (cx=0x7fffd9205000, args=..., construct=js::NO_CONSTRUCT) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:496 #74 0x00007fffe6ce28f4 in InternalCall (cx=0x7fffd9205000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:523 #75 0x00007fffe6ce29ae in js::Call (cx=0x7fffd9205000, fval=..., thisv=..., args=..., rval=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:542 #76 0x00007fffe72ad66c in JS_CallFunctionValue (cx=0x7fffd9205000, obj=..., fval=..., args=..., rval=...) at /home/ckerschb/source/mc/js/src/jsapi.cpp:2852 ---Type <return> to continue, or q <return> to quit--- #77 0x00007fffe1c1970f in nsFrameMessageManager::ReceiveMessage (this=0x7fffc1f3d560, aTarget=0x7fffc1fbc440, aTargetFrameLoader=0x0, aTargetClosed=false, aMessage=u"WebNavigation:LoadURI", aIsSync=false, aCloneData=0x7fffffffb760, aCpows=0x7fffffffb6e0, aPrincipal=0x0, aRetVal=0x0) at /home/ckerschb/source/mc/dom/base/nsFrameMessageManager.cpp:1100 #78 0x00007fffe1c17f4c in nsFrameMessageManager::ReceiveMessage (this=0x7fffc1f3d560, aTarget=0x7fffc1fbc440, aTargetFrameLoader=0x0, aMessage=u"WebNavigation:LoadURI", aIsSync=false, aCloneData=0x7fffffffb760, aCpows=0x7fffffffb6e0, aPrincipal=0x0, aRetVal=0x0) at /home/ckerschb/source/mc/dom/base/nsFrameMessageManager.cpp:910 #79 0x00007fffe3ab304c in mozilla::dom::TabChild::RecvAsyncMessage(nsString const&, nsTArray<mozilla::jsipc::CpowEntry>&&, IPC::Principal const&, mozilla::dom::ClonedMessageData const&) (this=0x7fffc2052800, aMessage=u"WebNavigation:LoadURI", aCpows=<unknown type in /home/ckerschb/source/mc-obj-ff-dbg/dist/bin/libxul.so, CU 0x13cd7f46, DIE 0x13f0acb7>, aPrincipal=..., aData=...) at /home/ckerschb/source/mc/dom/ipc/TabChild.cpp:2338 #80 0x00007fffe0f28459 in mozilla::dom::PBrowserChild::OnMessageReceived (this=0x7fffc2052860, msg__=...) at /home/ckerschb/source/mc-obj-ff-dbg/ipc/ipdl/PBrowserChild.cpp:2681 #81 0x00007fffe105f99d in mozilla::dom::PContentChild::OnMessageReceived (this=0x7ffff6926020, msg__=...) at /home/ckerschb/source/mc-obj-ff-dbg/ipc/ipdl/PContentChild.cpp:5162 #82 0x00007fffe3a71a75 in mozilla::dom::ContentChild::OnMessageReceived (this=0x7ffff6926020, aMsg=...) at /home/ckerschb/source/mc/dom/ipc/ContentChild.cpp:3659 #83 0x00007fffe09e6fa5 in mozilla::ipc::MessageChannel::DispatchAsyncMessage (this=0x7ffff6926148, aMsg=...) at /home/ckerschb/source/mc/ipc/glue/MessageChannel.cpp:2110 #84 0x00007fffe09e69af in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) (this=0x7ffff6926148, aMsg=<unknown type in /home/ckerschb/source/mc-obj-ff-dbg/dist/bin/libxul.so, CU 0x2ac89a3, DIE 0x2b5454c>) at /home/ckerschb/source/mc/ipc/glue/MessageChannel.cpp:2036 #85 0x00007fffe09e5ee4 in mozilla::ipc::MessageChannel::RunMessage (this=0x7ffff6926148, aTask=...) at /home/ckerschb/source/mc/ipc/glue/MessageChannel.cpp:1882 #86 0x00007fffe09e6240 in mozilla::ipc::MessageChannel::MessageTask::Run (this=0x7fffc1f643f0) at /home/ckerschb/source/mc/ipc/glue/MessageChannel.cpp:1915 #87 0x00007fffe014758e in mozilla::SchedulerGroup::Runnable::Run (this=0x7fffc1f6f1a0) at /home/ckerschb/source/mc/xpcom/threads/SchedulerGroup.cpp:396 #88 0x00007fffe016d0fe in nsThread::ProcessNextEvent (this=0x7fffdd21fa00, aMayWait=false, aResult=0x7fffffffc887) at /home/ckerschb/source/mc/xpcom/threads/nsThread.cpp:1039 #89 0x00007fffe0173275 in NS_ProcessNextEvent (aThread=0x7fffdd21fa00, aMayWait=false) at /home/ckerschb/source/mc/xpcom/threads/nsThreadUtils.cpp:521 #90 0x00007fffe09eb284 in mozilla::ipc::MessagePump::Run (this=0x7ffff69c9c90, aDelegate=0x7fffffffcc20) at /home/ckerschb/source/mc/ipc/glue/MessagePump.cpp:97 #91 0x00007fffe09ebc1e in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x7ffff69c9c90, aDelegate=0x7fffffffcc20) at /home/ckerschb/source/mc/ipc/glue/MessagePump.cpp:301 #92 0x00007fffe09394ae in MessageLoop::RunInternal (this=0x7fffffffcc20) at /home/ckerschb/source/mc/ipc/chromium/src/base/message_loop.cc:326 #93 0x00007fffe0939432 in MessageLoop::RunHandler (this=0x7fffffffcc20) at /home/ckerschb/source/mc/ipc/chromium/src/base/message_loop.cc:319 #94 0x00007fffe09393f6 in MessageLoop::Run (this=0x7fffffffcc20) at /home/ckerschb/source/mc/ipc/chromium/src/base/message_loop.cc:299 #95 0x00007fffe3f33c2c in nsBaseAppShell::Run (this=0x7fffd6199f60) at /home/ckerschb/source/mc/widget/nsBaseAppShell.cpp:158 #96 0x00007fffe69ccd2a in XRE_RunAppShell () at /home/ckerschb/source/mc/toolkit/xre/nsEmbedFunctions.cpp:866 #97 0x00007fffe09ebacb in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x7ffff69c9c90, aDelegate=0x7fffffffcc20) at /home/ckerschb/source/mc/ipc/glue/MessagePump.cpp:269 #98 0x00007fffe09394ae in MessageLoop::RunInternal (this=0x7fffffffcc20) at /home/ckerschb/source/mc/ipc/chromium/src/base/message_loop.cc:326 #99 0x00007fffe0939432 in MessageLoop::RunHandler (this=0x7fffffffcc20) at /home/ckerschb/source/mc/ipc/chromium/src/base/message_loop.cc:319 #100 0x00007fffe09393f6 in MessageLoop::Run (this=0x7fffffffcc20) at /home/ckerschb/source/mc/ipc/chromium/src/base/message_loop.cc:299 #101 0x00007fffe69cc6c8 in XRE_InitChildProcess (aArgc=15, aArgv=0x7fffffffcfa8, aChildData=0x7fffffffce63) at /home/ckerschb/source/mc/toolkit/xre/nsEmbedFunctions.cpp:691 #102 0x00007fffe69daef4 in mozilla::BootstrapImpl::XRE_InitChildProcess (this=0x7ffff69b70a8, argc=17, argv=0x7fffffffcfa8, aChildData=0x7fffffffce63) at /home/ckerschb/source/mc/toolkit/xre/Bootstrap.cpp:65 #103 0x0000000000405c82 in content_process_main (bootstrap=0x7ffff69b70a8, argc=17, argv=0x7fffffffcfa8) at /home/ckerschb/source/mc/browser/app/../../ipc/contentproc/plugin-container.cpp:63 #104 0x0000000000406282 in main (argc=18, argv=0x7fffffffcfa8, envp=0x7fffffffd040) at /home/ckerschb/source/mc/browser/app/nsBrowserApp.cpp:285

Olli, I looked at this yet a little closer and what I want to do is basically create a parity implementation of what's on mozilla-central at the moment. Even though it's a little hacky at the moment I would like to talk the approach over. Before doing the recursive call of > rv = targetDocShell->InternalLoad(aURI, I am storing the current mScriptGlobal on the targetdocshell, which I then feed into the loadinfo. That mScriptGlobal is then ultimately used as the context for content policy checks within the contentSecuritymanager. Using that approach the test test_contentpolicy_block_window.html works again correctly (besides the assertion failure in the interpreter, but bill is going to help me with that). If the load is blocked by content policies, then we would return the error value all the way back up and we could close the window again, similar to what happens at the moment if NS_ERROR_NO_CONTENT is returned. What do you think about that? That would allow us to remove the content policy check from within docshell and seems like a parity implementation of what we have right now on mozilla-central.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #16) > I feel different, at the moment we perform 2 (more or less identical) > content policy checks for docshell loads. Only when we're opening a new window, right?, and we need to do some extra check before opening a new window

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #18) > If the load is blocked by content policies, then we would return the error > value all the way back up and we could close the window again, But we don't want to open the window at all. Opening and then closing a window is rather bad from ux point of view. It is already bad that we occasionally open a new tab, then start download, and then close the tab. That is really behavior from user point of view.

Comment on attachment 8902854 [details] [diff] [review] bug_1331740_contentpolicy_docshell.patch I do agree that having sane context always is something to fix, but changing cp checks so that it can lead to opening and then immediately closing some window isn't something we should do, IMO.

I think we would have to make test_contentpolicy_block_window.html be a browser-chrome test to avoid the assertion. That way, the content policy would run with chrome privileges.

(In reply to Olli Pettay [:smaug] from comment #21) > I do agree that having sane context always is something to fix, but > changing cp checks so that it can lead to opening and then immediately > closing some window isn't something we should do, IMO. Olli, I was wondering if you would like to reconsider your opinion on this bug. I agree that opening and immediately closing a new window is semi optimal in case a content policy check would deny loading of the new window. However, we already do a very similar thing in case the URI to be loaded has no data associated with it [1]. We open a new window and then immediately close it. Once could view this use case to be similar. Here is my point why the trade off is worth considering: At the moment, for every docshell load, in particular every iframe load within a page, we perform two (more or less) identical content policy checks. I imagine we could squeeze out some performance by removing the content policy check within docshell and only performing one instead of two content policy checks. This is particularly relevant for any kind of content blockers. E.g. the content process code of AdblockPlus needs to query the parent for content policy decisions, which blocks the entire content process every time, which downgrades user experience and leads to the user perception that Firefox is slow. By removing the content policy check within docshell we could speed up that scenario. I agree it's a tradeoff, but one that we should reconsider, giving the advantages of removing the additional content policy check. What do you think? [1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10206-10214

If we pass the same data to the first and second content policy check, hopefully whoever implements a content policy has cached the recent result and can just quickly give the same result in both cases. I don't know how content policies work in WebExtensions world though. Do we need to bake in such cache to some WebExtension API implementation? And our internal content policy implementations sure better be fast enough to not cause performance issues. Opening and then closing a window immediately is, IMO, very bad for UX. The fact that we need to do that currently in some cases doesn't mean that we can do it even in more cases.

(In reply to Olli Pettay [:smaug] from comment #24) > If we pass the same data to the first and second content policy check, > hopefully whoever implements a content policy has cached the recent result > and can just quickly give the same result in both cases. Hopefully, as long as the null context in the second contentpolicy check does not somehow invalidate that cache. > I don't know how content policies work in WebExtensions world though. Do we > need to bake in such cache to some WebExtension API implementation? I don't know myself, we would have to ask someone who knows. > And our internal content policy implementations sure better be fast enough > to not cause performance issues. As already mentioned, I am pretty sure they are fast enough. > Opening and then closing a window immediately is, IMO, very bad for UX. The > fact that we need to do that currently in some cases doesn't mean that we > can do it even in more cases. Does that mean this bug becomes a WONTFIX, or should we at least minimize the damage by just calling the additional cp check within docshell for loads of TYPE_DOCUMENT. I suppose, we could pass the right context to the loadInfo so that at least iframe loads consult content policies once. Would you agree to that?

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #25) > (In reply to Olli Pettay [:smaug] from comment #24) > > If we pass the same data to the first and second content policy check, > > hopefully whoever implements a content policy has cached the recent result > > and can just quickly give the same result in both cases. > > Hopefully, as long as the null context in the second contentpolicy check > does not somehow invalidate that cache. But isn't this bug going to fix that null context > > Does that mean this bug becomes a WONTFIX, I thought this bug was mostly about passing non-null context > or should we at least minimize > the damage by just calling the additional cp check within docshell for loads > of TYPE_DOCUMENT. Not sure I understand what this means.

(In reply to Olli Pettay [:smaug] from comment #26) > > Hopefully, as long as the null context in the second contentpolicy check > > does not somehow invalidate that cache. > But isn't this bug going to fix that null context Initially, I wanted to remove the content policy check from within docshell entirely, pass the right context to the loadInfo and consult content policy for docshell loads (with the right context) only within asyncOpen2(). I agree that opening and immediately closing a new window, in case it's blocked by content policies is not that crisp, even though I personally would take that trade off. I think the new focus of this bug is; pass the right context to the loadInfo so we can consult content policies using the right context within asyncOpen2(), but also keeping the content policy check for the case when we are about to open a new window. Maybe we can filter exactly that case so all iframe loads don't need to consult content policies twice, but only within asyncOpen2(). Does that make sense what I am suggesting?

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #27) > Initially, I wanted to remove the content policy check from within docshell > entirely, pass the right context to the loadInfo and consult content policy > for docshell loads (with the right context) only within asyncOpen2(). I > agree that opening and immediately closing a new window, in case it's > blocked by content policies is not that crisp, even though I personally > would take that trade off. I wonder why you'd take that trade off. Isn't better ux after all what we should aim for. > > I think the new focus of this bug is; Well, the bug title hints about passing context, so not sure about the focus change ;) pass the right context to the loadInfo > so we can consult content policies using the right context within > asyncOpen2(), but also keeping the content policy check for the case when we > are about to open a new window. Maybe we can filter exactly that case so all > iframe loads don't need to consult content policies twice, but only within > asyncOpen2(). > > Does that make sense what I am suggesting? That sounds reasonable. I thought we didn't do the double check with iframe, only with new window case.

(In reply to Olli Pettay [:smaug] from comment #28) > > I think the new focus of this bug is; > Well, the bug title hints about passing context, so not sure about the focus > change ;) Sorry, I guess the bug title might be misleading > ContentSecurityManager needs to pass the correct context for TYPE_DOCUMENT loads (ake remove content policy check from docshell) it should rather be: Docshell should pass the right context for loads of TYPE_DOCUMENT to the loadInfo, so the contentSecuritymanager can pass it to contentPolicy checks. No change of focus :-) But to sum it up, I think that makes sense now: * Consult content policies within docshell for the cases when opening a new window * Pass the right context to the loadInfo for loads of TYPE_DOCUMENT * Consult content policies for the remaining docshell loads * Downside: Content policies for the new window load are consulted twice. Potentially we could add a new content policy type. e.g. TYPE_WINDOW_OPEN and use that for the content policy check within docshell.

As agreed, here is the patch. Eventually we could remove the HSTS Priming code right after the content policy check right after the content policy check within docshell and only do that HSTS Priming foo right after loadinfo creation, which would allow to remove some duplicate code. Up to you - thanks for your help with this! One last question, you sure we shouldn't label that bug as [qf]? In my opinion we should, but also up to you.

I haven't seen this stuff in any performance profiles. But perhaps I haven't looked at the right kind of profiles?

Comment on attachment 8903176 [details] [diff] [review] bug_1331740_contentpolicy_docshell.patch Does doesn't this basically (runtime) leak the global pointed by mSourceScriptGlobal if the opener window is closed before the window which was the target for the load. And how does this work if there are several other windows targeting loads to window Foo. Each of those other windows set Foo's mSourceScriptGlobal. And what is mSourceScriptGlobal for the initial load? (This is tricky. Right now I'm starting to think we should just use TabChild as the context for top level document loads, and add a new CP type for new window case and use opener script global there or something. But not sure.)

(In reply to Olli Pettay [:smaug] from comment #32) > Does doesn't this basically (runtime) leak the global pointed by > mSourceScriptGlobal if the opener window is closed before the window which > was the target for the load. I am not sure I can follow what you mean. Why would that be the case? > And how does this work if there are several other windows targeting loads to > window Foo. Each of those other windows set Foo's mSourceScriptGlobal. > And what is mSourceScriptGlobal for the initial load? My thinking was that one docshell can have one opener and that opener is stored within the loadInfo throughout the entire load process for that docshell URI. > (This is tricky. Right now I'm starting to think we should just use TabChild > as the context for top level document loads, and add a new CP type for new > window case and use opener script global there or something. But not sure.) I think I agree to that. Basically have TYPE_WINDOW for the cp check within docshell and for the loadinfo context we pass TabChild as the context? Mainly the reason I have the current setup is to generate some kind of parity implementation to the current checks.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #33) > (In reply to Olli Pettay [:smaug] from comment #32) > > Does doesn't this basically (runtime) leak the global pointed by > > mSourceScriptGlobal if the opener window is closed before the window which > > was the target for the load. > > I am not sure I can follow what you mean. Why would that be the case? > You have window A and B. window A targets a load to B, so static_cast<nsDocShell*>(targetDocShell.get())->mSourceScriptGlobal = mScriptGlobal; is executed, and B.mSourceScriptGlobal points to A's global. Now one closes A. Nothing releases its global.

(In reply to Olli Pettay [:smaug] from comment #34) > You have window A and B. window A targets a load to B, so > static_cast<nsDocShell*>(targetDocShell.get())->mSourceScriptGlobal = > mScriptGlobal; is executed, and B.mSourceScriptGlobal points to A's global. > Now one closes A. Nothing releases its global. Would using a weakPtr be of any help here or are we then running in danger that mSourceScriptGlobal might be null before we do the CP check?

Smaug, here is a summary of our IRC conversation (which is a potential path forward): * Only perform cp check within docshell for loads of TYPE_DOCUMENT when opening a new window (all other cases will be handled within asyncOpen2()). [For that load we could replace the cp type of TYPE_DOCUMENT with TYPE_WINDOW, which would make sense in my opinion.] * For top level loads we pass TabChild as the context to the loadinfo and ultimately to cp checks. This change causes this assertion [1] to fire, but I guess we could update that assertion to include the TabChild. Are we on the right track? [1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsContentPolicy.cpp#96

(In reply to Bill McCloskey (:billm) from comment #22) > I think we would have to make test_contentpolicy_block_window.html be a > browser-chrome test to avoid the assertion. That way, the content policy > would run with chrome privileges. Thanks Bill; I think using the new approach for this bug doesn't require us to rewrite the test in the end.

Comment on attachment 8903277 [details] [diff] [review] bug_1331740_contentpolicy_docshell.patch ># HG changeset patch ># User Christoph Kerschbaumer <ckerschb@christophkerschbaumer.com> ># Date 1504207992 -7200 ># Thu Aug 31 21:33:12 2017 +0200 ># Node ID d58fde88ebde34fcc9e0bb800b23047ca45e46f2 ># Parent fb22415719a9d971a2646fa2d1b74e134ca00c3d >Bug 1331740: Pass correct context for TYPE_DOCUMENT loads within docshell. r=smaug > >diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp >--- a/docshell/base/nsDocShell.cpp >+++ b/docshell/base/nsDocShell.cpp >@@ -9923,37 +9923,34 @@ nsDocShell::InternalLoad(nsIURI* aURI, > // an iframe since that's more common. > contentType = nsIContentPolicy::TYPE_INTERNAL_IFRAME; > } > } else { > contentType = nsIContentPolicy::TYPE_DOCUMENT; > isTargetTopLevelDocShell = true; > } > >- // If there's no targetDocShell, that means we are about to create a new >- // window (or aWindowTarget is empty). Perform a content policy check before >- // creating the window. >- if (!targetDocShell) { >+ // If there's no targetDocShell and the contentPolicyType is of TYPE_DOCUMENT, >+ // then we are creating a new window. You drop the "(or aWindowTarget is empty)" part here. Why? Yet you aren't checking whether aWindowTarget is non-empty. Look below how we check !aWindowTarget.IsEmpty() and create a new docshell if needed. Let's perform a content policy check >+ // before creating that new window. Please note for all other docshell loads >+ // content policy checks are performed within the contentSecurityManager when >+ // then channel is about to be openend. >+ if (!targetDocShell && contentType == nsIContentPolicy::TYPE_DOCUMENT) { So this may happen also on aWindowTarget.IsEmpty() cases. I think it should be enough to do this check really only on cases we're about to open a new window >+ if (XRE_IsContentProcess()) { >+ // In e10s the child process doesn't have access to the element that >+ // contains the browsing context (because that element is in the chrome >+ // process). So we just pass mScriptGlobal. >+ requestingContext = ToSupports(mScriptGlobal); > } else { >+ // This is for loading non-e10s tabs and toplevel windows of various >+ // sorts. >+ // For the toplevel window cases, requestingElement will be null. > requestingElement = mScriptGlobal->AsOuter()->GetFrameElementInternal(); > requestingContext = requestingElement; I wonder if we should make non-e10s work the same way as e10s. > > #ifdef DEBUG > if (requestingElement) { > // Get the docshell type for requestingElement. > nsCOMPtr<nsIDocument> requestingDoc = requestingElement->OwnerDoc(); > nsCOMPtr<nsIDocShell> elementDocShell = requestingDoc->GetDocShell(); >@@ -9987,37 +9984,16 @@ nsDocShell::InternalLoad(nsIURI* aURI, > > if (NS_FAILED(rv) || NS_CP_REJECTED(shouldLoad)) { > if (NS_SUCCEEDED(rv) && shouldLoad == nsIContentPolicy::REJECT_TYPE) { > return NS_ERROR_CONTENT_BLOCKED_SHOW_ALT; > } > > return NS_ERROR_CONTENT_BLOCKED; > } >- >- // If HSTS priming was set by nsMixedContentBlocker::ShouldLoad, and we >- // would block due to mixed content, go ahead and block here. If we try to >- // proceed with priming, we will error out later on. >- nsCOMPtr<nsIDocShell> docShell = NS_CP_GetDocShellFromContext(requestingContext); >- // When loading toplevel windows, requestingContext can be null. We don't >- // really care about HSTS in that situation, though; loads in toplevel >- // windows should all be browser UI. >- if (docShell) { >- nsIDocument* document = docShell->GetDocument(); >- NS_ENSURE_TRUE(document, NS_OK); >- >- HSTSPrimingState state = document->GetHSTSPrimingStateForLocation(aURI); >- if (state == HSTSPrimingState::eHSTS_PRIMING_BLOCK) { >- // HSTS Priming currently disabled for InternalLoad, so we need to clear >- // the location that was added by nsMixedContentBlocker::ShouldLoad >- // Bug 1269815 will address images loaded via InternalLoad >- document->ClearHSTSPrimingLocation(aURI); >- return NS_ERROR_CONTENT_BLOCKED; >- } >- } Why can we move this? Doesn't the move lead to opening a new window? >+ // If HSTS priming was set by nsMixedContentBlocker::ShouldLoad, and we >+ // would block due to mixed content, go ahead and block here. If we try to >+ // proceed with priming, we will error out later on. yeah, this is too late to block it. Now I realized our data: blocking is wrong too.

>+ if (!targetDocShell && contentType == nsIContentPolicy::TYPE_DOCUMENT) { I'm a little confused by that test. What is it trying to test? The correct "we are creating a new window" test is: if (!targetDocShell && !aWindowTarget.IsEmpty()) { because that indicates that we were given a target and failed to find the corresponding window. If aWindowTarget.IsEmpty(), then we're doing the load in ourselves, no matter what contentType is, so are definitely not creating a new window.

(In reply to Olli Pettay [:smaug] from comment #38) > >+ if (!targetDocShell && contentType == nsIContentPolicy::TYPE_DOCUMENT) { > So this may happen also on aWindowTarget.IsEmpty() cases. > I think it should be enough to do this check really only on cases we're > about to open a new window Thanks Smaug and Boris, that makes sense actually, but to make sure, in the case of creating a new window the content policy type always needs to be of TYPE_DOCUMENT, right? > >+ // This is for loading non-e10s tabs and toplevel windows of various > >+ // sorts. > >+ // For the toplevel window cases, requestingElement will be null. > > requestingElement = mScriptGlobal->AsOuter()->GetFrameElementInternal(); > > requestingContext = requestingElement; > I wonder if we should make non-e10s work the same way as e10s. I would rather not mess with that at this point. If needed, I am happy to file a follow up. > >- HSTSPrimingState state = document->GetHSTSPrimingStateForLocation(aURI); > Why can we move this? Doesn't the move lead to opening a new window? You are right, we should keep that part here to shortcut hsts priming issues. > Now I realized our data: blocking is wrong too. Yes, thanks for filing Bug 1395948, I'll take a look at that, but then there is also Bug 1394554, CC'ed you there as well. Different problems than what this bug tries to solve.

Comment on attachment 8904007 [details] [diff] [review] bug_1331740_contentpolicy_docshell.patch >- if (contentType == nsIContentPolicy::TYPE_DOCUMENT) { >- if (XRE_IsContentProcess()) { >- // In e10s the child process doesn't have access to the element that >- // contains the browsing context (because that element is in the chrome >- // process). So we just pass mScriptGlobal. >- requestingContext = ToSupports(mScriptGlobal); >- } else { >- // This is for loading non-e10s tabs and toplevel windows of various >- // sorts. >- // For the toplevel window cases, requestingElement will be null. >- requestingElement = mScriptGlobal->AsOuter()->GetFrameElementInternal(); >- requestingContext = requestingElement; >- } >+ if (XRE_IsContentProcess()) { >+ // In e10s the child process doesn't have access to the element that >+ // contains the browsing context (because that element is in the chrome >+ // process). So we just pass mScriptGlobal. >+ requestingContext = ToSupports(mScriptGlobal); > } else { >- requestingElement = mScriptGlobal->AsOuter()->GetFrameElementInternal(); >+ // This is for loading non-e10s tabs and toplevel windows of various >+ // sorts. >+ // For the toplevel window cases, requestingElement will be null. >+ nsCOMPtr<Element> requestingElement = >+ mScriptGlobal->AsOuter()->GetFrameElementInternal(); I don't quite understand why we need to pass requestingElement in this window opening case, but I guess that is what the old code did, so fine. > /** >+ * A C++ friendly version of the loadingContext for toplevel loads. >+ * >+ * Most likely you want to query the ownerDocument or LoadingNode >+ * and not this context only available for TYPE_DOCUMENT loads. >+ */ >+ [noscript, notxpcom, nostdcall, binaryname(TopLevelLoadingContext)] >+ nsISupports binaryTopLevelLoadingContext(); Could we call this (binary)ContextForTopLevelLoad and document that it is null in other cases. Some tests?

(In reply to Olli Pettay [:smaug] from comment #42) > I don't quite understand why we need to pass requestingElement in this > window opening case, but I guess that is what the old code did, so fine. Legacy, but I am happy to file a follow up if needed. > >+ nsISupports binaryTopLevelLoadingContext(); > Could we call this (binary)ContextForTopLevelLoad and document that it is > null in other cases. Yes, incorported into this patch. Carrying over r+. > Some tests? As discussed on IRC, we should have enough test coverage, in particular important is: * docshell/test/navigation/test_contentpolicy_block_window.html https://treeherder.mozilla.org/#/jobs?repo=try&revision=2088b86e1cd97224666a83f01c851c18272d62db

> in the case of creating a new window the content policy type always needs to be of TYPE_DOCUMENT, right? Yes, because we will have isTargetTopLevelDocShell set to true by this code: http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/docshell/base/nsDocShell.cpp#9899-9905

It seems that I didn't take out the assertion which was only meant for iframe loads for the old cp check within docshell. In the remaining cp check within docshell, we don't check iframe loads anymore, those are checked in the contentSecurityManager, hence we can remove that assertion (see underneath) from the docshell code. Please note that this assertion has already been added to the code where we create the new loadInfo. TRY looks good now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=73cf4a21cfd230a4f4729fafc2c320bfa582eff5&selectedJob=128587620 %---snip--- -#ifdef DEBUG - if (requestingElement) { - // Get the docshell type for requestingElement. - nsCOMPtr<nsIDocument> requestingDoc = requestingElement->OwnerDoc(); - nsCOMPtr<nsIDocShell> elementDocShell = requestingDoc->GetDocShell(); - - // requestingElement docshell type = current docshell type. - MOZ_ASSERT(mItemType == elementDocShell->ItemType(), - "subframes should have the same docshell type as their parent"); - } -#endif

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/807e9eadd5c1 Pass correct context for TYPE_DOCUMENT loads within docshell. r=smaug

So far we haven't seen any adverse effects in Thunderbird :-) Thanks again for the consideration.

(In reply to Jorg K (GMT+2) from comment #48) > So far we haven't seen any adverse effects in Thunderbird :-) Thanks again > for the consideration. That's good to know - thanks.