Put Blob URL online #12440

izgzhen · 2016-07-14T03:16:20Z

This PR connects the resource requests with file manager thread, including:

script_thread load request
image_cache_thread load request
XHR load request (the responding part code yet not implemented due to unfamiliarity with fetch standard, but the infra is here)

One notable change is the introduction of "long-live validity", to handle the case specified in WPT test FileAPI/blob/Blob-XHR-revoke.html.

r? @Manishearth

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #10539
There are tests for these changes

This change is

highfive · 2016-07-14T03:16:25Z

Heads up! This PR modifies the following files:

@KiChjang: components/net/filemanager_thread.rs, components/net_traits/blob_url_store.rs, components/net_traits/blob_url_store.rs, components/script/dom/url.rs, components/script/dom/xmlhttprequest.rs, components/net/blob_loader.rs, components/net_traits/filemanager_thread.rs, components/net_traits/filemanager_thread.rs, components/script/script_thread.rs, components/script/dom/htmlinputelement.rs, components/script/dom/blob.rs, components/net/image_cache_thread.rs

izgzhen · 2016-07-14T03:16:58Z

Still WIP, welcome comments! (esp. about "long-live")

Manishearth · 2016-07-14T13:49:35Z

components/net/filemanager_thread.rs

-                let _ = sender.send(Err(BlobURLStoreError::InvalidOrigin));
-            }
+        // Check origin format correctness
+        if Url::parse(&origin).is_ok() || origin == "file://" {


Why do we have a separate check for file://?

Because Url::parse is not happy with an origin equal to file://. It is kinda a hack (related to get_blob_origin, parse_blob_url etc. as well)

Manishearth · 2016-07-14T13:51:17Z

components/net/filemanager_thread.rs

+                origin: origin.clone(),
+                file_impl: FileImpl::Memory(blob_buf),
+                refs: AtomicUsize::new(1),
+                // Valid here since PromoteMemory implies URL creation


Not necessarily, yes? What if a child blob promotes the parent in-memory blob when it is converted to a URL?

We should have validity be a bool option on this function. Call it "valid_as_url" or something

Manishearth · 2016-07-14T13:51:53Z

components/net/image_cache_thread.rs

+                        if ref_url.scheme() == "blob" {
+                            let msg = FileManagerThreadMsg::LoadBlob(load_data,
+                                        LoadConsumer::Listener(response_target));
+                            self.resource_threads.send(msg).unwrap(); // XXX: unwrap?!


No unwrap, just let _ = ..

Manishearth · 2016-07-14T13:54:03Z

components/net_traits/blob_url_store.rs

+pub fn get_blob_origin(url: &Url) -> FileOrigin {
+    if url.scheme() == "file" {
+        // NOTE: by default this is "null" (Opaque), which is not ideal
+        "file://".to_string()


Aren't file URIs supposed to be considered opaque origin? What's wrong with using the opaque origin in the blob?

Well, FileOrigin is still a unicode-serialization of url::Origin, which, when it is file://, turned into "null"

A lot of things can be null, including file://, so I am trying to make this case more special here. (not a elegant solution though)

works for me

Manishearth · 2016-07-14T13:55:54Z

components/script/dom/xmlhttprequest.rs

-        core_resource_thread.send(Fetch(init, action_sender)).unwrap();
+
+        if init.url.scheme() == "blob" {
+            resource_threads.send(FileManagerThreadMsg::Fetch(init, action_sender)).unwrap(); // XXX: unwrap?


.expect("Resource thread shut down")

Manishearth · 2016-07-14T14:13:09Z

components/script/dom/xmlhttprequest.rs

@@ -368,6 +373,11 @@ impl XMLHttpRequestMethods for XMLHttpRequest {
                // Step 11 - abort existing requests
                self.terminate_ongoing_fetch();

+                // See WPT test: FileAPI/blob/Blob-XHR-revoke.html


So this is wrong. I think you're aware of this since you've marked it as a hack 😄

The correct behavior is that XHR queries for validity during open(), and is allowed to access the blob even after the URL validity expires, provided that it was allowed to during open(). Firefox returns an error on invalid blob URIs in open() (for both existing-but-revoked URIs and nonexistant URIs)

We should have a second refcount, which counts the number of XHRs holding on to this. When xhr calls open(), it should send a special message that checks validity and bumps this refcount if still valid; and the message will respond with the validity state (which is now just a bool). When the XHR gets a response or error, it un-bumps the refcount. Deleting an entry from the store is done when the refcounts and validity state are null. HTTP fetches will not fetch URLs with a false validity. New XHR requests will also fail on open() for invalid URIs. Existing XHRs will work fine.

The spec says nothing about this, however 😄

for now, don't include this hack, have a simple validity state and let the test fail. We'll work on clearing up the spec and only then implement it.

That would be good

Feel free to make a PR to wpt to remove this test, too, since it's unspecced and inconsistent across browsers. If this gets specced and/or becomes consistent, we can add it again.

We should probably create an issue on a spec somewhere, but right now I'm discussing with baku and we'll figure out which spec we care about.

So I will keep a comment in tree for now.

Manishearth · 2016-07-14T14:14:05Z

components/script/script_thread.rs

+            let msg = FileManagerThreadMsg::LoadBlob(net_load_data, LoadConsumer::Listener(response_target));
+            self.resource_threads.send(msg).unwrap();
+        } else {
+            let msg = CoreResourceMsg::Load(net_load_data, LoadConsumer::Listener(response_target), None);


The CoreResource handler should be doing the delegation to the file manager thread, not script_task

Yep, that is one way. But I remembered that we discussed this before and thought CoreResource thread handler might be too busy thus clogged?

No, it shouldn't get clogged easily since it just spawns threads for each message.

It could get clogged in cases where we pool, but in that case we shouldn't be giving File fetches any priority over other fetches anyway.

I see. But that will incur some broader restructuring over net process interface so I wonder if I should do it here (since we split filemanagermsg out from core one before)

Manishearth · 2016-07-14T14:16:13Z

tests/wpt/mozilla/tests/mozilla/blob_url_upload_ref.html

+<body>
+    <img src="test.jpg" id="image">
+    <input type="file" id="file-input"">
+</body>


newline at eof for both tests

izgzhen · 2016-07-15T00:10:04Z

components/net/resource_thread.rs

@@ -548,6 +554,7 @@ impl CoreResourceManager {
            },
            "data" => from_factory(data_loader::factory),
            "about" => from_factory(about_loader::factory),
+            "blob" => blob_loader::factory(self.filemanager_chan.clone()),


@Manishearth I think this might get it right now

Manishearth · 2016-07-15T06:17:25Z

components/net/blob_loader.rs

-                                           None, name.as_bytes().to_vec())
-            ]
-        });
+pub fn factory(filemanager_chan: IpcSender<FileManagerThreadMsg>)


Perhaps we should take a reference (arc) to the file store instead?

Then again, we already have a filemanager thread, so meh.

Maybe handle the msg to FileManager will make resource_thread.rs look less formidable 😄

Manishearth · 2016-07-15T09:47:50Z

components/net/blob_loader.rs

+                if let Ok(chan) =
+                    start_sending_sniffed_opt(start_chan, metadata, classifier,
+                                              &blob_buf.bytes, load_data.context.clone()) {
+                    let _ = chan.send(Payload(blob_buf.bytes));


This sends the payload twice, start_sending_sniffed_opt already does this

But partial_body is only used to classify the content as I can tell from the code :/

Manishearth · 2016-07-15T09:49:30Z

One small issue, then update test expectations, and should be good to go.

Manishearth · 2016-07-15T10:25:25Z

@bors-servo try

There should be some new tests passing

bors-servo · 2016-07-15T10:25:27Z

⌛ Trying commit 932a2a8 with merge a7ec87c...


        Auto merge of #12440 - izgzhen:blob-online, r=<try>

@Manishearth

Put Blob URL online This PR connects the resource requests with file manager thread, including: + `script_thread` load request + `image_cache_thread` load request + XHR load request (the responding part code yet not implemented due to unfamiliarity with fetch standard, but the infra is here) One notable change is the introduction of "long-live validity", to handle the case specified in WPT test FileAPI/blob/Blob-XHR-revoke.html. r? @Manishearth  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #10539 - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12440)

izgzhen · 2016-07-15T10:28:16Z

@Manishearth Actually I first thought it will send the Payload for me and didn't write that line code. Should I file an issue on this potentially misleading naming?

bors-servo · 2016-07-15T10:38:35Z

💔 Test failed - linux-dev


        Auto merge of #12440 - izgzhen:blob-online, r=<try>

@Manishearth

Put Blob URL online This PR connects the resource requests with file manager thread, including: + `script_thread` load request + `image_cache_thread` load request + XHR load request (the responding part code yet not implemented due to unfamiliarity with fetch standard, but the infra is here) One notable change is the introduction of "long-live validity", to handle the case specified in WPT test FileAPI/blob/Blob-XHR-revoke.html. r? @Manishearth  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #10539 - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12440)

bors-servo · 2016-07-15T11:20:57Z

💔 Test failed - linux-dev

izgzhen · 2016-07-15T11:24:39Z

@bors-servo try

Fixed tidy (Feeling like homu would come to GMT 0800 and bite me)

bors-servo · 2016-07-15T11:24:42Z

⌛ Trying commit 60b116b with merge 4cc4bc3...


        Auto merge of #12440 - izgzhen:blob-online, r=<try>

@Manishearth

Put Blob URL online This PR connects the resource requests with file manager thread, including: + `script_thread` load request + `image_cache_thread` load request + XHR load request (the responding part code yet not implemented due to unfamiliarity with fetch standard, but the infra is here) One notable change is the introduction of "long-live validity", to handle the case specified in WPT test FileAPI/blob/Blob-XHR-revoke.html. r? @Manishearth  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #10539 - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12440)

bors-servo · 2016-07-15T12:27:07Z

💔 Test failed - linux-rel

highfive · 2016-07-15T12:27:10Z

  ▶ PASS [expected FAIL] /FileAPI/url/url_xmlhttprequest_img.html

  ▶ OK [expected TIMEOUT] /workers/constructors/Worker/Blob-url.html

  ▶ Unexpected subtest result in /workers/constructors/Worker/Blob-url.html:
  └ PASS [expected TIMEOUT] Worker supports Blob url


        Put Blob URL online

izgzhen · 2016-07-15T12:34:46Z

@bors-servo try

WPT tests updated

bors-servo · 2016-07-15T12:34:49Z

⌛ Trying commit fdc3a8e with merge db547d0...


        Auto merge of #12440 - izgzhen:blob-online, r=<try>

@Manishearth

Put Blob URL online This PR connects the resource requests with file manager thread, including: + `script_thread` load request + `image_cache_thread` load request + XHR load request (the responding part code yet not implemented due to unfamiliarity with fetch standard, but the infra is here) One notable change is the introduction of "long-live validity", to handle the case specified in WPT test FileAPI/blob/Blob-XHR-revoke.html. r? @Manishearth  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #10539 - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12440)

bors-servo · 2016-07-15T13:35:18Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

Manishearth · 2016-07-15T13:39:21Z

@bors-servo r+

Now that most of the functionality exists, could you go through the list of tests that still fail and ensure that there aren't any tests there that shouldn't be (tests which should pass in Servo but fail)? I'm a bit concerned by the small number of fixed tests in each PR, but it looks like WPT has very few File tests to begin with 😄 so I don't think that's due to imperfections in your changes.

Perhaps we should add more.

bors-servo · 2016-07-15T13:39:22Z

📌 Commit fdc3a8e has been approved by Manishearth

bors-servo · 2016-07-15T13:39:24Z

⌛ Testing commit fdc3a8e with merge 0e5893d...


        Auto merge of #12440 - izgzhen:blob-online, r=Manishearth

@Manishearth

Put Blob URL online This PR connects the resource requests with file manager thread, including: + `script_thread` load request + `image_cache_thread` load request + XHR load request (the responding part code yet not implemented due to unfamiliarity with fetch standard, but the infra is here) One notable change is the introduction of "long-live validity", to handle the case specified in WPT test FileAPI/blob/Blob-XHR-revoke.html. r? @Manishearth  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #10539 - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12440)

izgzhen · 2016-07-15T13:44:18Z

@Manishearth as I peeked a few days ago, most of the failed tests are due to ArrayBuffer. But yes, I should really consider adding more tests :)

bors-servo · 2016-07-15T14:40:13Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

nox · 2016-07-18T08:38:14Z

Would be great to explain why the file manager now needs to be passed to new_core_resource_thread, this isn't made clear in the corresponding commit.

izgzhen · 2016-07-18T14:55:57Z

@nox Because the ResourceManager will be responsible to forward the blob URL loading request to file manager as here. So we need to grab a reference to it in the initialisation.

highfive assigned Manishearth Jul 14, 2016

highfive added the S-awaiting-review label Jul 14, 2016

izgzhen force-pushed the izgzhen:blob-online branch from 6889bd0 to 56d43db Jul 14, 2016

Manishearth reviewed Jul 14, 2016
View changes

tests/wpt/mozilla/tests/mozilla/blob_url_upload_ref.html

<body>

<img src="test.jpg" id="image">

<input type="file" id="file-input"">

</body>

This comment has been minimized.

Sign in to view

Manishearth Jul 14, 2016
Member

newline at eof for both tests

izgzhen force-pushed the izgzhen:blob-online branch 3 times, most recently from 5a10e14 to fef71bd Jul 14, 2016

izgzhen reviewed Jul 15, 2016
View changes

izgzhen force-pushed the izgzhen:blob-online branch from fef71bd to 456e2d3 Jul 15, 2016

Manishearth reviewed Jul 15, 2016
View changes

izgzhen force-pushed the izgzhen:blob-online branch from 456e2d3 to 932a2a8 Jul 15, 2016

Manishearth reviewed Jul 15, 2016
View changes

highfive added the S-tests-failed label Jul 15, 2016

izgzhen force-pushed the izgzhen:blob-online branch from 932a2a8 to a364a37 Jul 15, 2016

highfive removed the S-tests-failed label Jul 15, 2016

highfive added the S-tests-failed label Jul 15, 2016

izgzhen force-pushed the izgzhen:blob-online branch from a364a37 to 60b116b Jul 15, 2016

highfive removed the S-tests-failed label Jul 15, 2016

highfive added the S-tests-failed label Jul 15, 2016

Put Blob URL online

Loading status checks…

fdc3a8e

izgzhen force-pushed the izgzhen:blob-online branch from 60b116b to fdc3a8e Jul 15, 2016

highfive removed the S-tests-failed label Jul 15, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 15, 2016

bors-servo merged commit fdc3a8e into servo:master Jul 15, 2016
1 of 2 checks passed

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jul 15, 2016

servo / servo

Join GitHub today

Put Blob URL online #12440

Put Blob URL online #12440

Conversation

izgzhen commented Jul 14, 2016 • edited

highfive commented Jul 14, 2016

izgzhen commented Jul 14, 2016 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Jul 15, 2016

Manishearth commented Jul 15, 2016

bors-servo commented Jul 15, 2016

izgzhen commented Jul 15, 2016

bors-servo commented Jul 15, 2016

bors-servo commented Jul 15, 2016

izgzhen commented Jul 15, 2016

bors-servo commented Jul 15, 2016

bors-servo commented Jul 15, 2016

highfive commented Jul 15, 2016

izgzhen commented Jul 15, 2016

bors-servo commented Jul 15, 2016

bors-servo commented Jul 15, 2016

Manishearth commented Jul 15, 2016

bors-servo commented Jul 15, 2016

bors-servo commented Jul 15, 2016

izgzhen commented Jul 15, 2016

bors-servo commented Jul 15, 2016

nox commented Jul 18, 2016

izgzhen commented Jul 18, 2016

izgzhen commented Jul 14, 2016 •

edited

izgzhen commented Jul 14, 2016 •

edited