Fix revoke blob url #24685

shnmorimoto · 2019-11-07T17:22:19Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #24290 (GitHub issue number if applicable)

There are tests for these changes OR
These changes do not require tests because ___

highfive · 2019-11-07T17:22:27Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @nox (or someone else) soon.

highfive · 2019-11-07T17:22:28Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/url.rs
@KiChjang: components/net/filemanager_thread.rs, components/script/dom/url.rs, components/net_traits/filemanager_thread.rs

highfive · 2019-11-07T17:22:31Z

Warning

These commits modify net and script code, but no tests are modified. Please consider adding a test!

jdm · 2019-11-07T17:29:26Z

@bors-servo try=wpt

bors-servo · 2019-11-07T17:29:31Z

⌛ Trying commit 999818e with merge 970a12d...


        Auto merge of #24685 - shnmorimoto:fix_revoke_blob_url, r=<try>

Fix revoke blob url  fix #24290 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #24290 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2019-11-07T18:05:22Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-community-tc, status-taskcluster
State: approved= try=True

jdm · 2019-11-07T18:09:31Z

Hmm, the goal of #24290 was to get tests/wpt/web-platform-tests/FileAPI/url/url-with-xhr.any.js and tests/wpt/web-platform-tests/FileAPI/url/url-with-fetch.any.js passing, so it's curious that there is no change in test results.

jdm · 2019-11-07T18:11:03Z

components/net/filemanager_thread.rs

-            FileManagerThreadMsg::RevokeBlobURL(id, origin, sender) => {
-                let _ = sender.send(self.store.set_blob_url_validity(false, &id, &origin));
+            FileManagerThreadMsg::RevokeBlobURL(url, sender) => {
+                if let Ok((id, origin)) = parse_blob_url(&url) {


Oh - we still are throwing away the fragment and query string from the URL, which is why the tests continue to fail. We need to explicitly deal with those.

Thanks for the review.
I will check and fix it!

shnmorimoto · 2019-11-10T08:38:29Z

@jdm

I have two quesrtions.

1. About parse_blob_url

parse_blob_url gets uuid and origin from url.

ex.

if "blob:http://web-platform.test:8000/8da840d9fed84875be1e896b8b2cbcc9" is given as url.
And parse_blob_url gets 8da840d9fed84875be1e896b8b2cbcc9 as uuid and http://web-platform.test:8000 as origin.

And, CreateObjectURL seems to store uuid as key.(because it call set_blob_url_validity.)

servo/components/net/filemanager_thread.rs

Line 679 in e5689df

id: &Uuid,

So I think that it is correct that RevokeObjectURL uses uuid as key and uses parse_blob_url.

2. About failed tests.

I ran the test in my local(macOS Mojave 10.14.6).
And, I got the following results.

> ./mach test-wpt tests/wpt/web-platform-tests/FileAPI/url/url-with-xhr.any.js    

...

web-platform-test
~~~~~~~~~~~~~~~~~
Ran 30 checks (28 subtests, 2 tests)
Expected results: 30
Unexpected results: 0
OK
 0:05.78 INFO Closing logging queue
 0:05.78 INFO queue closed

> ./mach test-wpt tests/wpt/web-platform-tests/FileAPI/url/url-with-fetch.any.js

...

0:04.77 TEST_END: Test OK. Subtests passed 10/15. Unexpected 1
FAIL Revoke blob URL after calling fetch, fetch should succeed - promise_test: Unhandled rejection with value: object "TypeError: Network error occurred"
 0:04.77 INFO No more tests
 0:04.78 INFO Closing logging queue
 0:04.78 INFO queue closed
 0:04.80 TEST_END: Test OK. Subtests passed 10/15. Unexpected 1
FAIL Revoke blob URL after calling fetch, fetch should succeed - promise_test: Unhandled rejection with value: object "TypeError: Network error occurred"
 0:04.80 INFO No more tests
 0:04.81 INFO Closing logging queue
 0:04.81 INFO queue closed
 0:04.82 INFO Got 2 unexpected results
 0:04.82 SUITE_END

web-platform-test
~~~~~~~~~~~~~~~~~
Ran 32 checks (30 subtests, 2 tests)
Expected results: 30
Unexpected results: 2
  subtest: 2 (2 fail)

Unexpected Results
------------------
/FileAPI/url/url-with-fetch.any.html
  FAIL Revoke blob URL after calling fetch, fetch should succeed - promise_test: Unhandled rejection with value: object "TypeError: Network error occurred"
/FileAPI/url/url-with-fetch.any.worker.html
  FAIL Revoke blob URL after calling fetch, fetch should succeed - promise_test: Unhandled rejection with value: object "TypeError: Network error occurred"

Is this the error you said?

And, I tried adding the following to url-with-fetch.any.js.

  ...
  const result = fetch_should_succeed(t, url).then(text => {
    assert_equals(text, blob_contents);
  });

  // added code
  for (i = 0; i < 1000; i ++) {
    console.log("aaaa");
  }

  // Revoke the object URL. fetch should have already resolved the blob URL.
  URL.revokeObjectURL(url);

  return result;
}, 'Revoke blob URL after calling fetch, fetch should succeed');

And, I got the following result.

> ./mach test-wpt tests/wpt/web-platform-tests/FileAPI/url/url-with-fetch.any.js
...

 0:05.20 TEST_END: Test OK. Subtests passed 11/15. Unexpected 0
 0:05.20 INFO No more tests
 0:05.21 INFO Closing logging queue
 0:05.21 INFO queue closed
 0:05.21 INFO Got 0 unexpected results
 0:05.21 SUITE_END

web-platform-test
~~~~~~~~~~~~~~~~~
Ran 32 checks (30 subtests, 2 tests)
Expected results: 32
Unexpected results: 0
OK
 0:05.24 INFO Closing logging queue
 0:05.24 INFO queue closed

So, I think that the cause of the error is the fetch timing.

Is this correct?

nox · 2019-11-12T12:14:40Z

r? @jdm

jdm · 2019-11-12T19:26:50Z

@shnmorimoto With respect to the test output, when a test says:

Ran 30 checks (28 subtests, 2 tests)
Expected results: 30
Unexpected results: 0
OK
 0:05.78 INFO Closing logging queue
 0:05.78 INFO queue closed

That means that the test results matched the expected results. For that test, they are present in this file, which shows that various subtests are currently expected to fail. By making the changes I recommend, our goal is to make the subtests pass and remove these expected failures.

As for the question about parse_blob_url, rather than my original suggestion (make the RevokeBlobURL message accept a URL argument), I think we should modify parse_blob_url to return an Err if the parsed URL contains a fragment or query string. This should achieve the same goal - revoking a blob URL that has a fragment (#foo) attached to it should not cause the blob to be revoked.

shnmorimoto · 2019-11-13T02:26:27Z

@jdm I misunderstood the test output. Thank you for the detailed explanation.

I think we should modify parse_blob_url to return an Err if the parsed URL contains a fragment or query string. This should achieve the same goal - revoking a blob URL that has a fragment (#foo) attached to it should not cause the blob to be revoked.

I understand. I will fix it that way.

shnmorimoto · 2019-11-17T18:11:23Z

@jdm I've fixed it. For fragment, there are following test.

 promise_test(t => {
    const url = URL.createObjectURL(blob);

    return fetch_should_succeed(t, url + '#fragment').then(text => {
      assert_equals(text, blob_contents);
    });
  }, fetch_method + ' with a fragment should succeed');

So, It is only checked in RevokeObjectURL.

What do you think?

jdm · 2019-11-17T18:37:44Z

Good find. I agree!

shnmorimoto · 2019-11-17T19:50:17Z

Thanks! I fixed it as follows and already pushed.

if let Ok(url) = ServoUrl::parse(&url) {
    if url.fragment().is_none() && origin == get_blob_origin(&url) {

I ran the test locally and confirmed that the results changed.

web-platform-test
~~~~~~~~~~~~~~~~~
Ran 30 checks (28 subtests, 2 tests)
Expected results: 24
Unexpected results: 6
  subtest: 6 (6 pass)

Unexpected Results
------------------
/FileAPI/url/url-with-xhr.any.worker.html
  UNEXPECTED-PASS Only exact matches should revoke URLs, using XHR
  UNEXPECTED-PASS Appending a query string should cause XHR to fail
  UNEXPECTED-PASS Appending a path should cause XHR to fail
/FileAPI/url/url-with-xhr.any.html
  UNEXPECTED-PASS Only exact matches should revoke URLs, using XHR
  UNEXPECTED-PASS Appending a query string should cause XHR to fail
  UNEXPECTED-PASS Appending a path should cause XHR to fail
 0:05.07 INFO Closing logging queue
 0:05.08 INFO queue closed

jdm · 2019-11-17T20:04:56Z

Please include the updated test results in this PR, in that case :)

jdm · 2019-11-17T20:05:44Z

You can use ./mach test-wpt tests/wpt/web-platform-tests/FileAPI as the base command in order to avoid having to run thousands of unnecessary tests, too!

shnmorimoto · 2019-11-17T20:46:40Z

Oh, Sorry. I updated test results.

I still get unexpected results locally.

> ./mach test-wpt tests/wpt/web-platform-tests/FileAPI                                                                                                                  fix_revoke_blob_url_2 ✱
Running 56 tests in web-platform-tests

  ▶ Unexpected subtest result in /FileAPI/url/url-with-fetch.any.worker.html:
  │ FAIL [expected PASS] Revoke blob URL after calling fetch, fetch should succeed
  └   → promise_test: Unhandled rejection with value: object "TypeError: Network error occurred"

  ▶ Unexpected subtest result in /FileAPI/url/url-in-tags-revoke.window.html:
  │ FAIL [expected TIMEOUT] Opening a blob URL in a new window immediately before revoking it works.
  │   → assert_equals: expected (string) "test_frame_OK" but got (undefined) undefined
  │
  │ win.onload<@http://web-platform.test:8000/FileAPI/url/url-in-tags-revoke.window.js:52:5
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1908:25
  └ Test.prototype.step_func_done/<@http://web-platform.test:8000/resources/testharness.js:1948:32

  ▶ Unexpected subtest result in /FileAPI/url/url-with-fetch.any.html:
  │ FAIL [expected PASS] Revoke blob URL after calling fetch, fetch should succeed
  └   → promise_test: Unhandled rejection with value: object "TypeError: Network error occurred"

Ran 56 tests finished in 117.0 seconds.
  • 53 ran as expected. 0 tests skipped.
  • 3 tests had unexpected subtest results

But, I get same results with master branch too.
So, I ignored it this time.

bors-servo · 2019-11-17T21:20:21Z

☔ The latest upstream changes (presumably #24761) made this pull request unmergeable. Please resolve the merge conflicts.


        check same-origin and url against the blob beign revoked

shnmorimoto · 2019-11-18T14:31:56Z

I did rebase and resolved merge conflicts.

jdm · 2019-11-18T17:13:26Z

@bors-servo r+
Thanks for fixing this!

bors-servo · 2019-11-18T17:13:27Z

📌 Commit 2d995ba has been approved by jdm

bors-servo · 2019-11-19T00:32:03Z

⌛ Testing commit 2d995ba with merge cc4d3c0...


        Auto merge of #24685 - shnmorimoto:fix_revoke_blob_url, r=jdm

Fix revoke blob url  fix #24290 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #24290 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2019-11-19T01:59:58Z

💔 Test failed - status-taskcluster

jdm · 2019-11-19T02:02:17Z

@bors-servo retry

#24762

bors-servo · 2019-11-19T02:41:18Z

💣 Failed to start rebuilding: 405 Not Allowed

bors-servo · 2019-11-19T02:41:23Z

⌛ Testing commit 2d995ba with merge d553158...


        Auto merge of #24685 - shnmorimoto:fix_revoke_blob_url, r=jdm

Fix revoke blob url  fix #24290 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #24290 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2019-11-19T03:20:43Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing d553158 to master...

highfive assigned nox Nov 7, 2019

highfive added the S-awaiting-review label Nov 7, 2019

jdm reviewed Nov 7, 2019

View changes

highfive assigned jdm and unassigned nox Nov 12, 2019

jdm added S-needs-code-changes and removed S-awaiting-review labels Nov 12, 2019

shnmorimoto force-pushed the shnmorimoto:fix_revoke_blob_url branch from 999818e to bc9de39 Nov 17, 2019

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 17, 2019

shnmorimoto force-pushed the shnmorimoto:fix_revoke_blob_url branch from bc9de39 to 570adf2 Nov 17, 2019

highfive added the S-needs-rebase label Nov 17, 2019

shnmorimoto force-pushed the shnmorimoto:fix_revoke_blob_url branch from 570adf2 to be3cc5c Nov 17, 2019

check same-origin and url against the blob beign revoked

Loading status checks…

2d995ba

shnmorimoto force-pushed the shnmorimoto:fix_revoke_blob_url branch from be3cc5c to 2d995ba Nov 18, 2019

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Nov 18, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 19, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Nov 19, 2019

bors-servo merged commit 2d995ba into servo:master Nov 19, 2019
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 19, 2019

servo / servo

Fix revoke blob url #24685

Fix revoke blob url #24685

shnmorimoto commented Nov 7, 2019

highfive commented Nov 7, 2019

highfive commented Nov 7, 2019

highfive commented Nov 7, 2019

jdm commented Nov 7, 2019

bors-servo commented Nov 7, 2019

bors-servo commented Nov 7, 2019

jdm commented Nov 7, 2019

This comment has been minimized.

This comment has been minimized.

shnmorimoto commented Nov 10, 2019

nox commented Nov 12, 2019

jdm commented Nov 12, 2019

shnmorimoto commented Nov 13, 2019

shnmorimoto commented Nov 17, 2019

jdm commented Nov 17, 2019

shnmorimoto commented Nov 17, 2019

jdm commented Nov 17, 2019

jdm commented Nov 17, 2019

shnmorimoto commented Nov 17, 2019

bors-servo commented Nov 17, 2019

shnmorimoto commented Nov 18, 2019

jdm commented Nov 18, 2019

bors-servo commented Nov 18, 2019

bors-servo commented Nov 19, 2019

bors-servo commented Nov 19, 2019

jdm commented Nov 19, 2019

bors-servo commented Nov 19, 2019

bors-servo commented Nov 19, 2019

bors-servo commented Nov 19, 2019

servo / servo

Join GitHub today

Fix revoke blob url #24685

Fix revoke blob url #24685

Conversation

shnmorimoto commented Nov 7, 2019

highfive commented Nov 7, 2019

highfive commented Nov 7, 2019

highfive commented Nov 7, 2019

jdm commented Nov 7, 2019

bors-servo commented Nov 7, 2019

bors-servo commented Nov 7, 2019

jdm commented Nov 7, 2019

This comment has been minimized.

jdm Nov 7, 2019

This comment has been minimized.

shnmorimoto Nov 8, 2019

shnmorimoto commented Nov 10, 2019

1. About parse_blob_url

2. About failed tests.

nox commented Nov 12, 2019

jdm commented Nov 12, 2019

shnmorimoto commented Nov 13, 2019

shnmorimoto commented Nov 17, 2019

jdm commented Nov 17, 2019

shnmorimoto commented Nov 17, 2019

jdm commented Nov 17, 2019

jdm commented Nov 17, 2019

shnmorimoto commented Nov 17, 2019

bors-servo commented Nov 17, 2019

shnmorimoto commented Nov 18, 2019

jdm commented Nov 18, 2019

bors-servo commented Nov 18, 2019

bors-servo commented Nov 19, 2019

bors-servo commented Nov 19, 2019

jdm commented Nov 19, 2019

bors-servo commented Nov 19, 2019

bors-servo commented Nov 19, 2019

bors-servo commented Nov 19, 2019