Jonas, please have a look at that WIP patch and let me know if this is the approach you would like to take to wrap/mediate the context. Might be worth adding that helper class somewhere else, because I suppose we would like to use that mediation technique in other places too, e.g: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.cpp#620 Please also note that we can't remove ShouldLoadScript() at this point because other code [1] is still using it. Once we get there, we can remove those bits. Also, I don't have a strong opinion about the name of 'contextMediator', we can rename and add a generic comment what that code is doing, wherever this code might end up. [1] http://mxr.mozilla.org/mozilla-central/source/dom/xul/XULDocument.cpp#3254

Comment on attachment 8648273 [details] [diff] [review] bug_1194526_asyncopen2_scriptloader.patch Review of attachment 8648273 [details] [diff] [review]: ----------------------------------------------------------------- r=me with that fixed. ::: dom/base/nsScriptLoader.cpp @@ +281,5 @@ > + NS_ENSURE_TRUE(window, NS_ERROR_NULL_POINTER); > + nsIDocShell *docshell = window->GetDocShell(); > + nsCOMPtr<nsIInterfaceRequestor> prompter(do_QueryInterface(docshell)); > + > + nsSecurityFlags securityFlags = aRequest->mCORSMode == CORS_NONE put everything after the '=' on a new line (indented) in order to keep 80 columns. @@ +282,5 @@ > + nsIDocShell *docshell = window->GetDocShell(); > + nsCOMPtr<nsIInterfaceRequestor> prompter(do_QueryInterface(docshell)); > + > + nsSecurityFlags securityFlags = aRequest->mCORSMode == CORS_NONE > + ? nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS Actually, due to how workers handle data:-urls, you should use SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL here. @@ +292,4 @@ > nsCOMPtr<nsIChannel> channel; > + nsresult rv = NS_NewChannel(getter_AddRefs(channel), > + aRequest->mURI, > + mDocument, We should use aRequest->mElement here if it's non-null. And then make nsContentSecurityManager grab the document as context when calling nsIContentPolicies. @@ +344,3 @@ > nsCOMPtr<nsIStreamListener> listener = loader.get(); > > + return channel->AsyncOpen2(listener); Simply pass 'loader' here and get rid of the temporary. @@ +1690,5 @@ > +} > + > +contextMediator::~contextMediator() > +{ > +} Is there a reason not to simply use the default dtor? I.e. remove this and the dtor from the class definition? ::: dom/base/nsScriptLoader.h @@ +533,5 @@ > bool mWasEnabled; > nsRefPtr<nsScriptLoader> mLoader; > }; > > +class contextMediator : public nsIStreamLoaderObserver Stick this in the .cpp file. And uppercase the class-name. I.e. ContextMediator.

(In reply to Jonas Sicking (:sicking) from comment #2) > > nsCOMPtr<nsIChannel> channel; > > + nsresult rv = NS_NewChannel(getter_AddRefs(channel), > > + aRequest->mURI, > > + mDocument, > > We should use aRequest->mElement here if it's non-null. And then make > nsContentSecurityManager grab the document as context when calling > nsIContentPolicies. NS_NewChannel requests type nsINode, but aRequest->mElement is of type nsIScriptElement [1] which does not inherit nsINode. Jonas, how would you like to have that nandled? [1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.h#110

use do_QI

(In reply to Jonas Sicking (:sicking) from comment #4) > use do_QI How would that ever return something non null? Even if 'aRequest->mElement.get()' is non null, I can not QI an interface which that object [1] does not inherit, or am I missing something? http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIScriptElement.h#27

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5) > (In reply to Jonas Sicking (:sicking) from comment #4) > > use do_QI > > How would that ever return something non null? Even if > 'aRequest->mElement.get()' is non null, I can not QI an interface which that > object [1] does not inherit, or am I missing something? Jonas, I ended up using QI, so the code in question looks like this: > nsCOMPtr<nsINode> context = > aRequest->mElement ? do_QueryInterface(aRequest->mElement) > : do_QueryInterface(mDocument); To my surprise, this works just fine, can you please enlighten me why? I updated everything else, but this behavior really surprises me. > > +contextMediator::~contextMediator() > > +{ > > +} > > Is there a reason not to simply use the default dtor? Yes, there is: > error: static_assert failed "Reference-counted class ContextMediator should not have a public destructor. Make this class's destructor non-public"

Comment on attachment 8648455 [details] [diff] [review] bug_1194526_asyncopen2_scriptloader.patch Review of attachment 8648455 [details] [diff] [review]: ----------------------------------------------------------------- Looks great. Only thing that I worry about is our handling for workers. In particular, do we still enforce that things like the |new Worker| is same-origin, and that |new Worker("data:...")| still run with a null principal. I think that's the case for both of them, but it'd be good to ensure that we have tests. ::: dom/base/nsScriptLoader.cpp @@ +278,5 @@ > + NS_DECL_NSISTREAMLOADEROBSERVER > + > +private: > + virtual ~ContextMediator() {} > + nsCOMPtr<nsIStreamLoaderObserver> mObserver; I think you can make this an nsRefPtr<nsScriptLoader> instead. That way the scriptloader doesn't have to inherit nsIStreamLoaderObserver, and if we ever remove the aContext argument from nsIStreamLoaderObserver, that wouldn't break this code. Up to you. @@ +308,5 @@ > } > > + nsCOMPtr<nsINode> context = > + aRequest->mElement ? do_QueryInterface(aRequest->mElement) > + : do_QueryInterface(mDocument); no need to QI mDocument to nsINode since nsINode is a baseclass of nsIDocument. So normal casting works well. I.e. something like: nsCOMPtr<nsINode> context; if (aRequest->mElement) { context = do_QI(aRequest->mElement); } else { context = mDocument; }

> Yes, there is: > > error: static_assert failed "Reference-counted class ContextMediator should not have a public destructor. Make this class's destructor non-public" Ooh. That's very cool. You could still inline the dtor to save a few lines of code, but up to you.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #9) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a97e95463b3 I already thought that this might cause trouble. In the current version we 'always' use the documents principal for security checks, but the context is depending on whether 'aRequest->mElement' is null or not, see: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#231 I don't think there is an easy way to handle that in our current setup of loadInfo. Jonas, what do you think?

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10) > (In reply to Christoph Kerschbaumer [:ckerschb] from comment #9) > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a97e95463b3 > > I already thought that this might cause trouble. In the current version we > 'always' use the documents principal for security checks, but the context is > depending on whether 'aRequest->mElement' is null or not, see: > http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#231 > > I don't think there is an easy way to handle that in our current setup of > loadInfo. Jonas, what do you think? Actually, it's even more pressing for CheckLoadURIWithPrincipal: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#256

In fact we were missing the 'allow-chrome' bits [1] that we just added for bug 1195162, hence I marked bug 1195162 blocking this one. Carrying over r+ from Jonas. [1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#257

Chatted with Jonas over IRC and we should pass the internalContentPolicyType to NS_CheckContentPolicy within nsScriptSecuritymanager. Just added that fix. Carrying over r+ from Jonas. However, we are still facing some issues here: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=78a3898ca7f1

Mike, I am not quite sure what's going on here, but it seems the patch from this bug causes your test to fall apart. Since you wrote the test I was wondering if you could provide any insights on what might go wrong here. See comment 13 for a link to the TRY push [failing test is within bc(7)] and also a stacktrace underneath. 28 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_aboutTabCrashed.js | Uncaught exception - at resource://testing-common/BrowserTestUtils.jsm:483 - Error: <xul:browser> needs to be remote in order to crash Stack trace: this.BrowserTestUtils.crashBrowser<@resource://testing-common/BrowserTestUtils.jsm:483:1 crashTabTestHelper/<@chrome://mochitests/content/browser/browser/base/content/test/general/browser_aboutTabCrashed.js:163:11 crashTabTestHelper@chrome://mochitests/content/browser/browser/base/content/test/general/browser_aboutTabCrashed.js:152:1 test_send_all@chrome://mochitests/content/browser/browser/base/content/test/general/browser_aboutTabCrashed.js:275:9 crashTabTestHelper@chrome://mochitests/content/browser/browser/base/content/test/general/browser_aboutTabCrashed.js:152:1 test_send_all@chrome://mochitests/content/browser/browser/base/content/test/general/browser_aboutTabCrashed.js:275:9 Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:21 this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7 Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11 this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7 Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:451:5 Tester_execTest@chrome://mochikit/content/browser-test.js:754:9 Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:676:7 SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:735:59 29 INFO Leaving test test_send_all

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14) > Mike, I am not quite sure what's going on here, but it seems the patch from > this bug causes your test to fall apart. Since you wrote the test I was > wondering if you could provide any insights on what might go wrong here. See > comment 13 for a link to the TRY push [failing test is within bc(7)] and > also a stacktrace underneath. > > > 28 INFO TEST-UNEXPECTED-FAIL | > browser/base/content/test/general/browser_aboutTabCrashed.js | Uncaught > exception - at resource://testing-common/BrowserTestUtils.jsm:483 - Error: > <xul:browser> needs to be remote in order to crash > Stack trace: > > this.BrowserTestUtils.crashBrowser<@resource://testing-common/ > BrowserTestUtils.jsm:483:1 > > crashTabTestHelper/<@chrome://mochitests/content/browser/browser/base/ > content/test/general/browser_aboutTabCrashed.js:163:11 > > crashTabTestHelper@chrome://mochitests/content/browser/browser/base/content/ > test/general/browser_aboutTabCrashed.js:152:1 > > test_send_all@chrome://mochitests/content/browser/browser/base/content/test/ > general/browser_aboutTabCrashed.js:275:9 > > crashTabTestHelper@chrome://mochitests/content/browser/browser/base/content/ > test/general/browser_aboutTabCrashed.js:152:1 > > test_send_all@chrome://mochitests/content/browser/browser/base/content/test/ > general/browser_aboutTabCrashed.js:275:9 > Handler.prototype.process@resource://gre/modules/Promise.jsm -> > resource://gre/modules/Promise-backend.js:937:21 > this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> > resource://gre/modules/Promise-backend.js:813:7 > > Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise. > jsm -> resource://gre/modules/Promise-backend.js:744:11 > this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> > resource://gre/modules/Promise-backend.js:776:7 > Promise.prototype.then@resource://gre/modules/Promise.jsm -> > resource://gre/modules/Promise-backend.js:451:5 > Tester_execTest@chrome://mochikit/content/browser-test.js:754:9 > > Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:676:7 > > SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome:// > mochikit/content/tests/SimpleTest/SimpleTest.js:735:59 > 29 INFO Leaving test test_send_all Hey - so I think there are a few issues here: It looks as if the orange you were hitting in your try push is not the one that you've posted in comment 14. I suspect that you were running the test locally for comment 14, and ran the mochitest without --e10s. browser_aboutTabCrashed.js must run with --e10s, since tabs can only crash with --e10s. The failures that you're seeing in the try push look like an intermittent that I flushed out in bug 1211799 (which, as of this writing, has not yet landed on mozilla-central). I've pushed to try for you with those patches applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a55064267b2

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #16) > Hey - so I think there are a few issues here: > The failures that you're seeing in the try push look like an intermittent > that I flushed out in bug 1211799 (which, as of this writing, has not yet > landed on mozilla-central). > > I've pushed to try for you with those patches applied: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a55064267b2 Hey Mike, thanks for the update and thanks for fixing. TRY looks good, there is only one other issue, but that is not related to your test at all :-)

Benjamin, Jonas, maybe one of you can help me reason about why test: > caps/tests/mochitest/test_bug292789.html is failing on treeherder [1]. Interestingly it passes locally on OSX and Linux. The part in the test that is causing trouble is [2]: > is(typeof XPInstallConfirm, "undefined", > "content should not be able to load <script> from chrome://mozapps"); The test loads: > "chrome://mozapps/content/xpinstall/xpinstallConfirm.js which is not accessible from content [3] so it should not be allowed to load and hence 'typeof XPInstallConfirm' should be 'undefined'. I get that behavior locally and that seems correct to me. Why it returns 'object' on Treeherder is the part that I don't understand. Any ideas? [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a55064267b2 [2] http://mxr.mozilla.org/mozilla-central/source/caps/tests/mochitest/test_bug292789.html?force=1#34 [3] http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/jar.mn#35

Lot's of debug statements, let's see if that provides any hints what to look out for: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7185e9156644

Only other thing I remembered (other than add lots of printfs and run through try again) is if you might have an uninitialized variable somewhere.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d10d929b7278 That's odd, locally I do get a call do the contentSecuritymanager when loading 'xpinstallConfirm.js', but on treeherder I don't see that call. > doContentSecurityCheck { > channelURI: chrome://mozapps/content/xpinstall/xpinstallConfirm.js > loadingPrincipal: http://mochi.test:8888/tests/caps/tests/mochitest/test_bug292789.html > triggeringPrincipal: http://mochi.test:8888/tests/caps/tests/mochitest/test_bug292789.html > contentPolicyType: 2 > securityMode: 8 > initalSecurityChecksDone: no > enforceSecurity: no > } Jonas, could it be some kind of caching problem?

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #21) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=d10d929b7278 > > That's odd, locally I do get a call do the contentSecuritymanager when > loading 'xpinstallConfirm.js', but on treeherder I don't see that call. > > > doContentSecurityCheck { > > channelURI: chrome://mozapps/content/xpinstall/xpinstallConfirm.js > > loadingPrincipal: http://mochi.test:8888/tests/caps/tests/mochitest/test_bug292789.html > > triggeringPrincipal: http://mochi.test:8888/tests/caps/tests/mochitest/test_bug292789.html > > contentPolicyType: 2 > > securityMode: 8 > > initalSecurityChecksDone: no > > enforceSecurity: no > > } > > Jonas, could it be some kind of caching problem? You are absolutely right, as per our IRC conversation. Thanks for the hint - TRY is green now - going to land that right away!

There's still nsIContentPolicy checks in the scriptloader. Those should be removed, right?

(In reply to Jonas Sicking (:sicking) from comment #26) > There's still nsIContentPolicy checks in the scriptloader. Those should be > removed, right? Yes, they need to be removed, I will get to that soon.