Closed Bug 1218433 Opened 5 years ago Closed 5 years ago

Use AsyncOpen2 in dom/workers/ScriptLoader.cpp

Categories

(Core :: DOM: Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(2 files, 5 obsolete files)

Follow up of 1211967 Comment 17.
Depends on: 1211967
Attached patch patch (obsolete) — Splinter Review
can you please review this patch? I removed some of the checks you suggested. Not all because they would break existing tests.
Attachment #8678956 - Flags: review?(mozilla)
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'
Attachment #8678956 - Flags: review?(mozilla)
Duplicate of this bug: 1206955
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.
Attachment #8678956 - Flags: review-
Attached patch patch (obsolete) — Splinter Review
Attachment #8678956 - Attachment is obsolete: true
Attachment #8679479 - Flags: review?(jonas)
Attachment #8679479 - Flags: review?(mozilla)
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 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
Attachment #8679479 - Flags: review?(mozilla) → review+
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
Did Jonas' suggestion in Comment 9 fix the issue?
Flags: needinfo?(mozilla)
Summary: Use AsyncOpen2 in ScriptLoader.cpp → Use AsyncOpen2 in dom/workers/ScriptLoader.cpp
> 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.
Flags: needinfo?(jonas)
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.
Flags: needinfo?(jonas)
(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...
Attached patch patch (obsolete) — Splinter Review
The issue was that we don't consider the result value of LoadScript.
Attachment #8679479 - Attachment is obsolete: true
Attachment #8682154 - Flags: review?(jonas)
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.
Attachment #8682154 - Flags: review?(jonas) → review+
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.
Flags: needinfo?(jonas)
Attached patch WPT fix (obsolete) — Splinter Review
Attachment #8684550 - Flags: review?(jonas)
Duplicate of this bug: 1194523
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.
Attachment #8684550 - Flags: review?(jonas) → review+
Attached patch patch 1Splinter Review
Attachment #8682154 - Attachment is obsolete: true
Attached patch patch 2 (obsolete) — Splinter Review
Attachment #8684550 - Attachment is obsolete: true
Patches ready to land.
Keywords: checkin-needed
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.
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.
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.)
Flags: needinfo?(amarchesini)
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
Flags: needinfo?(amarchesini)
Attached patch part 2 - WPTSplinter Review
All the changes in the mochitests have been already reviewed by Jonas.
Attachment #8686094 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Attachment #8687927 - Flags: review?(Ms2ger)
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.
Attachment #8687927 - Flags: review?(Ms2ger) → review+
Flags: needinfo?(amarchesini)
Depends on: 1241888
Depends on: 1260961
Depends on: 1260388
You need to log in before you can comment on or make changes to this bug.