1194526 - Use channel->ascynOpen2 in dom/base/nsScriptLoader.cpp

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194526_asyncopen2_scriptloader.patch

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 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(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

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

use do_QI

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(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

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194526_asyncopen2_scriptloader.patch

(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 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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;
}

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> 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.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a97e95463b3

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(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?

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(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

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194526_asyncopen2_scriptloader.patch

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

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194526_asyncopen2_scriptloader.patch

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

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 15

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a55064267b2

Comment 16

[Mike Conley (:mconley) (:⚙️)]

(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

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

(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 :-)

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

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?

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

(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!

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8a9f98b9650

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b8a9f98b9650

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b8a9f98b9650

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

(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.