1218433 - Use AsyncOpen2 in dom/workers/ScriptLoader.cpp

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Andrea Marchesini [:baku]]

Follow up of 1211967 Comment 17.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

can you please review this patch? I removed some of the checks you suggested. Not all because they would break existing tests.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8678956 [details] [diff] [review]
patch

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

Hey Baku, what tests are failing exactly?

::: dom/workers/ScriptLoader.cpp
@@ +154,5 @@
>      // (and other URLs that inherit their principal from their
>      // creator.)
>      rv = principal->CheckMayLoad(uri, false, true);
>      NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_SECURITY_ERR);
>    }

Please remove NS_CheckContentLoadPolicy, we do that within asyncOpen2().

@@ +159,4 @@
>  
>    aLoadFlags |= nsIChannel::LOAD_CLASSIFY_URI;
> +  uint32_t secFlags = aIsMainScript ? nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS
> +                                    : nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS;

In fact we should be able to completely remove all of those security checks from the callsite. You are using SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS for main scripts, hence you can also remove 'principal->CheckMayLoad'

Comment 4

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

Comment on attachment 8678956 [details] [diff] [review]
patch

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

::: dom/workers/ScriptLoader.cpp
@@ +159,4 @@
>  
>    aLoadFlags |= nsIChannel::LOAD_CLASSIFY_URI;
> +  uint32_t secFlags = aIsMainScript ? nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS
> +                                    : nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS;

We do want to ensure that data: URIs don't inherit. Using the SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS flag means that they do. Unfortunately we don't have a SEC_REQUIRE_SAME_ORIGIN_DATA_IS_NULL since workers is an odd one out, and one that we should probably change to align with the spec.

@@ +616,2 @@
>  
> +    ScriptLoadInfo& loadInfo = mLoadInfos[aIndex];

Nit: No need for this temporary.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 6

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

Comment on attachment 8679479 [details] [diff] [review]
patch

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

::: dom/workers/ScriptLoader.cpp
@@ +1225,5 @@
> +LoaderListener::OnStreamComplete(nsIStreamLoader* aLoader, nsISupports* aContext,
> +                              nsresult aStatus, uint32_t aStringLen,
> +                              const uint8_t* aString)
> +{
> +  return mRunnable->OnStreamComplete(aLoader, mIndex, aStatus, aStringLen, aString);

Nit: Might as well inline this and OnStartRequest in the class declaration.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8679479 [details] [diff] [review]
patch

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

Looks good to me and fixes web-platform tests in addition - nice!

::: dom/workers/ScriptLoader.cpp
@@ +132,5 @@
>                        NS_ERROR_DOM_SECURITY_ERR);
>  
>      if (!isChrome && !isResource) {
>        return NS_ERROR_DOM_SECURITY_ERR;
>      }

Since we are already touching this file we might as well eliminate those hardcoded schemes and use URI_IS_UI_RESOURCE protocol flag? This use case seems like exactly what it's designed for, as far as I can tell.

Please add a comment like
// A DebuggerScript needs to be a local resource like chrome: or resource:

bool isUIResource = false;
rv = NS_URIChainHasFlags(uri, nsIProtocolHandler::URI_IS_UI_RESOURCE, &isUIResource);
NS_ENSURE_SUCCESS(rv, rv);

if (!isUIResource) {
  return NS_ERROR_DOM_SECURITY_ERR;
}

@@ +611,3 @@
>    {
>      AssertIsOnMainThread();
> +    MOZ_ASSERT(aIndex < mLoadInfos.Length());

Wouldn't a different out of bounds check be more appropriate, something that returns an error for release builds?

@@ +1223,5 @@
> +
> +NS_IMETHODIMP
> +LoaderListener::OnStreamComplete(nsIStreamLoader* aLoader, nsISupports* aContext,
> +                              nsresult aStatus, uint32_t aStringLen,
> +                              const uint8_t* aString)

nit: wrong indentation of arguments

Comment 8

[Andrea Marchesini [:baku]]

The current setup breaks a couple a few mochitests:
https://treeherder.mozilla.org/logviewer.html#?job_id=13136743&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=13136753&repo=try

Comment 9

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

If I understand the stack correctly, it's when we're running the test at [1] that we fail. And specifically when that test is running the line at [2].

The test looks correct. That *should* fail. The page should at that time have a null principal, and it's trying to load a worker at a http URL. That means that we should be using nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED and that should fail with the given principal.


Maybe the problem is that we call AsyncOpen2 asynchronously? I.e. we don't call it directly from the Worker constructor.

In which case I guess we'll just fire an error event rather than load the worker?

So maybe we should change the test to not look for an exception, but to look for an exception or an error event?


[1] http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/general/test_storagePermissionsAccept.html?force=1#27
[2] http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/general/frameStoragePrevented.html?force=1#18

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Did Jonas' suggestion in Comment 9 fix the issue?

Comment 11

[Andrea Marchesini [:baku]]

> In which case I guess we'll just fire an error event rather than load the
> worker?

onerror is not called at all. I changed the test but the worker ctor doesn't fail, and it's onerror is not triggered.
I can dig more deeply in the code but maybe somebody else is more familiar with it.

Comment 12

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

Most likely we simply fail silently, but that's obviously not good. We should simply treat this as a network error.

I wonder how we handle this for "subworkers". Ideally we would handle things the same for toplevel workers.

Adding khuey who I think is the only other person that might be familiar with this code.

Comment 13

[Andrea Marchesini [:baku]]

(In reply to Jonas Sicking (:sicking) from comment #12)
> Most likely we simply fail silently, but that's obviously not good. We
> should simply treat this as a network error.

Good catch! This was the issue. /me changing the code...

Comment 14

[Andrea Marchesini [:baku]]

Attachment: patch

The issue was that we don't consider the result value of LoadScript.

Comment 15

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

Comment on attachment 8682154 [details] [diff] [review]
patch

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

Thanks!

::: dom/workers/ScriptLoader.cpp
@@ +127,5 @@
> +    bool isUIResource = false;
> +    rv = NS_URIChainHasFlags(uri, nsIProtocolHandler::URI_IS_UI_RESOURCE,
> +                             &isUIResource);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;

Nit: I like NS_ENSURE_SUCCESS(rv, rv); a lot more. I know others do not, but so far none of the counter proposals have seemed better. Expanding to three lines of code is definitely worse IMO.

@@ +1231,5 @@
> +                                 nsresult aStatus, uint32_t aStringLen,
> +                                 const uint8_t* aString)
> +{
> +  return mRunnable->OnStreamComplete(aLoader, mIndex, aStatus, aStringLen, aString);
> +}

I still think you can inline these in the class definition. Up to you though.

Comment 16

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=632902119bd0

The patch breaks some WPTs. I don't know if it's because the tests are wrong, the spec is wrong or what.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: WPT fix

Comment 19

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

Comment on attachment 8684550 [details] [diff] [review]
WPT fix

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

r=me with that fixed.

::: testing/web-platform/tests/mixed-content/generic/common.js
@@ +82,5 @@
>   */
>  function bindEvents(element, resolveEventName, rejectEventName) {
>    element.eventPromise = new Promise(function(resolve, reject) {
>      element.addEventListener(resolveEventName  || "load", resolve);
> +    element.onerror = function(e) { e.preventDefault(); reject(); }

It doesn't look right to simply ignore the rejectEventName.

Maybe do

element.addEventListener(rejectEventName || "error",
                         function(e) { e.preventDefault(); reject(); } );

::: testing/web-platform/tests/workers/Worker_cross_origin_security_err.htm
@@ +4,5 @@
>  <script src="/resources/testharnessreport.js"></script>
>  <div id=log></div>
>  <script>
> +async_test(function(t) {
> +  var w = new Worker("ftp://example.org/support/WorkerBasic.js");

Should probably allow for both throwing and firing onerror here.

So:

try { new Worker(...);
      e.onerror = t.step_func_done(function(e) {
	assert_true(e instanceof ErrorEvent); })
 }
catch () { t.step_func_done(function(e) {
	   assert_true(true); }) }

(or some such. Sorry, it's really hard to enter code in this tiny review window.)

::: testing/web-platform/tests/workers/constructors/SharedWorker/same-origin.html
@@ +24,5 @@
>  }, "data_url");
> +
> +async_test(function(t) {
> +  var w = new SharedWorker('javascript:""', '');
> +  w.onerror = t.step_func_done(function(e) { assert_true(e instanceof ErrorEvent); });

Same with all of these. Probably need a helper function which checks for both sync and async errors.

::: testing/web-platform/tests/workers/constructors/Worker/same-origin.html
@@ +22,5 @@
>  }, "data_url");
>  
> +async_test(function(t) {
> +  var w = new Worker('about:blank');
> +  w.onerror = t.step_func_done(function(e) { assert_true(e instanceof ErrorEvent); });

Same here.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: patch 1

Comment 21

[Andrea Marchesini [:baku]]

Attachment: patch 2

Comment 22

[Andrea Marchesini [:baku]]

Patches ready to land.

Comment 23

[Andrea Marchesini [:baku]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5d8b8744419f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/50af9a9ba6d9

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=17150110&repo=mozilla-inbound

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

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Looks like you also upset a few desktop tests: 
https://treeherder.mozilla.org/logviewer.html#?job_id=17151561&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=17151570&repo=mozilla-inbound

Comment 26

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

The dom/security/test/csp/test_child-src_worker-redirect.html failure looks like it just needs to be changed to accept error messages rather than exceptions. Probably true for the other ones too.

Note that for mochitests it's probably better to have them look for *only* async errors, since we don't want them to throw.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8dcbfd14f1e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a3092c456c0

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=89f3fe720292 / https://hg.mozilla.org/integration/mozilla-inbound/rev/89f3fe720292

Push causing test failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5a3092c456c0

Failures, e.g.

M(3)

07:34:30     INFO -  [Child 3937] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805E0006: file /builds/slave/m-in-l64-d-0000000000000000000/build/src/dom/security/nsContentSecurityManager.cpp, line 392
07:34:30     INFO -  [Child 3937] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805E0006: file /builds/slave/m-in-l64-d-0000000000000000000/build/src/netwerk/protocol/http/HttpChannelChild.cpp, line 1761
07:34:30     INFO -  [Child 3937] WARNING: 'NS_FAILED(rv)', file /builds/slave/m-in-l64-d-0000000000000000000/build/src/dom/workers/ScriptLoader.cpp, line 892
07:34:30     INFO -  [Child 3937] WARNING: 'NS_FAILED(rv)', file /builds/slave/m-in-l64-d-0000000000000000000/build/src/dom/workers/ScriptLoader.cpp, line 781
07:34:30     INFO -  [Child 3937] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805E0006: file /builds/slave/m-in-l64-d-0000000000000000000/build/src/dom/security/nsContentSecurityManager.cpp, line 392
07:34:30     INFO -  [Child 3937] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805E0006: file /builds/slave/m-in-l64-d-0000000000000000000/build/src/netwerk/protocol/http/HttpChannelChild.cpp, line 1761
07:34:30     INFO -  [Child 3937] WARNING: 'NS_FAILED(rv)', file /builds/slave/m-in-l64-d-0000000000000000000/build/src/dom/workers/ScriptLoader.cpp, line 892
07:34:30     INFO -  [Child 3937] WARNING: 'NS_FAILED(rv)', file /builds/slave/m-in-l64-d-0000000000000000000/build/src/dom/workers/ScriptLoader.cpp, line 781
07:34:30     INFO -  [Child 3937] WARNING: 'rv.Failed()', file /builds/slave/m-in-l64-d-0000000000000000000/build/src/dom/workers/WorkerPrivate.cpp, line 498
07:34:30     INFO -  TEST-INFO | started process screentopng
07:34:35     INFO -  TEST-INFO | screentopng: exit 0
07:34:35     INFO -  422 INFO TEST-UNEXPECTED-FAIL | dom/security/test/csp/test_child-src_worker-redirect.html | CSP child-src worker test other-src-worker_redir-same - got "Error: Failed to load script (nsresult = 0x805e0006)", expected "blocked"
07:34:35     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
07:34:35     INFO -      recvMessage@dom/security/test/csp/test_child-src_worker-redirect.html:89:1
07:34:35     INFO -      EventListener.handleEvent*@dom/security/test/csp/test_child-src_worker-redirect.html:98:7
07:34:35     INFO -  [Child 3937] WARNING: 'rv.Failed()', file /builds/slave/m-in-l64-d-0000000000000000000/build/src/dom/workers/WorkerPrivate.cpp, line 498

W(2)
TEST-UNEXPECTED-ERROR | /content-security-policy/child-src/child-src-worker-blocked.sub.html | Error: Failed to load script (nsresult = 0x805e0006)

Android M(10)

349 INFO TEST-UNEXPECTED-FAIL | dom/security/test/csp/test_child-src_worker-redirect.html | CSP child-src worker test other-src-worker_redir-same - got "Error: Failed to load script (nsresult = 0x805e0006)", expected "blocked"
365 INFO TEST-UNEXPECTED-FAIL | dom/security/test/csp/test_child-src_worker.html | Test timed out.
372 INFO TEST-UNEXPECTED-FAIL | dom/security/test/csp/test_child-src_worker_data.html | Test timed out.

Comment 29

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/285852de5cd5

Comment 30

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Since we seem to be intentionally violating the spec here, can we please get a bug filed and the test changes reverted? This is what the expectation files are for. (Also, the Worker constructor test certainly shouldn't use the SharedWorker constructor.)

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/325a67608df0
https://hg.mozilla.org/integration/mozilla-inbound/rev/22360424ed15

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Had to backout this for W(1) and W(2) errors:

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

http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-linux/1447588713/mozilla-inbound_ubuntu32_vm_test-web-platform-tests-1-bm01-tests1-linux32-build23.txt.gz

http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-linux-debug/1447588713/mozilla-inbound_ubuntu32_vm-debug_test-web-platform-tests-2-bm08-tests1-linux32-build61.txt.gz

06:22:34     INFO - TEST-START | /content-security-policy/child-src/child-src-worker-blocked.sub.html
06:22:35     INFO - TEST-PASS | /content-security-policy/child-src/child-src-worker-blocked.sub.html | Expecting alerts: ["PASS"] 
06:22:35     INFO - TEST-PASS | /content-security-policy/child-src/child-src-worker-blocked.sub.html | Violation report status OK. 
06:22:35     INFO - TEST-UNEXPECTED-ERROR | /content-security-policy/child-src/child-src-worker-blocked.sub.html | Error: Failed to load script (nsresult = 0x805e0006)
06:22:35     INFO - TEST-INFO took 1091ms
06:22:35  WARNING - u'runner_teardown' ()
06:22:35     INFO - Setting up ssl
06:22:35     INFO - PROCESS | certutil | 
06:22:35     INFO - PROCESS | certutil | 
06:22:35     INFO - PROCESS | certutil | 
06:22:35     INFO - Certificate Nickname                                         Trust Attributes
06:22:35     INFO -                                                              SSL,S/MIME,JAR/XPI
06:22:35     INFO - 
06:22:35     INFO - web-platform-tests                                           CT,, 
06:22:35     INFO - 
06:22:36     INFO - Starting runner

Comment 33

[Andrea Marchesini [:baku]]

Attachment: part 2 - WPT

All the changes in the mochitests have been already reviewed by Jonas.

Comment 34

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8687927 [details] [diff] [review]
part 2 - WPT

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

wpt changes seem fine; no comment on the dom/security changes.

Comment 35

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=243fb2e04e19

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5418ca0e0378
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1cc8cc0444c0

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

WPT busted like https://treeherder.mozilla.org/logviewer.html#?job_id=17343133&repo=mozilla-inbound when this landed.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/85237019374b

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05e6b00163eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c6b3811aeb7

Comment 38

[Pulsebot]

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

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/05e6b00163eb
https://hg.mozilla.org/mozilla-central/rev/6c6b3811aeb7
https://hg.mozilla.org/mozilla-central/rev/af9a4942a9c4