Use AsyncOpen2 in dom/workers/ScriptLoader.cpp

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected, firefox45 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

3 years ago
Follow up of 1211967 Comment 17.
(Assignee)

Updated

3 years ago
Depends on: 1211967
(Assignee)

Comment 1

3 years ago
Created attachment 8678956 [details] [diff] [review]
patch

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

Comment 5

3 years ago
Created attachment 8679479 [details] [diff] [review]
patch
Attachment #8678956 - Attachment is obsolete: true
Attachment #8679479 - Flags: review?(jonas)
(Assignee)

Updated

3 years ago
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
(Assignee)

Comment 11

3 years ago
> 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)
(Assignee)

Comment 13

3 years ago
(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...
(Assignee)

Comment 14

3 years ago
Created attachment 8682154 [details] [diff] [review]
patch

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+
(Assignee)

Comment 16

3 years ago
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)
(Assignee)

Comment 17

3 years ago
Created attachment 8684550 [details] [diff] [review]
WPT fix
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+
(Assignee)

Comment 20

3 years ago
Created attachment 8686093 [details] [diff] [review]
patch 1
Attachment #8682154 - Attachment is obsolete: true
(Assignee)

Comment 21

3 years ago
Created attachment 8686094 [details] [diff] [review]
patch 2
Attachment #8684550 - Attachment is obsolete: true
(Assignee)

Comment 22

3 years ago
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.)
(Assignee)

Updated

3 years ago
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)
(Assignee)

Comment 33

3 years ago
Created attachment 8687927 [details] [diff] [review]
part 2 - WPT

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+

Comment 39

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/05e6b00163eb
https://hg.mozilla.org/mozilla-central/rev/6c6b3811aeb7
https://hg.mozilla.org/mozilla-central/rev/af9a4942a9c4
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Assignee)

Updated

3 years ago
Flags: needinfo?(amarchesini)

Updated

3 years ago
Depends on: 1241888

Updated

3 years ago
Depends on: 1260961

Updated

3 years ago
Depends on: 1260388
You need to log in before you can comment on or make changes to this bug.