Implement Blob URL's DOM interfaces #11716

izgzhen · 2016-06-10T19:10:32Z

Implement the two functions in URL to create/revoke Blob URLs, and related code to approximate our proposed design to make things work together.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix part of #10539, related to #11131

There are tests for these changes OR

This change is

highfive · 2016-06-10T19:10:35Z

Heads up! This PR modifies the following files:

@KiChjang: components/net/filemanager_thread.rs, components/script/lib.rs, components/net_traits/blob_url_store.rs, components/net_traits/blob_url_store.rs, components/script/dom/url.rs, components/script/dom/webidls/URL.webidl, components/script/dom/workerglobalscope.rs, components/net_traits/filemanager_thread.rs, components/net_traits/filemanager_thread.rs, components/script/blob_url_store.rs, components/script/dom/window.rs

highfive · 2016-06-10T19:10:36Z

Warning

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

izgzhen · 2016-06-10T19:11:03Z

I am still not very sure about how to deal with this: the net thread need the Origin, where might it come from? The script thread?

There are also other holes in the code, I might fix them later in this PR or following ones.

Manishearth · 2016-06-10T19:31:38Z

For now just send it from the script thread and add a comment saying that net should actually be determining it from the pipeline id. Fetch/http_loader does this too right now. Its wrong, but I wouldn't bother fixing it for the purpose of this pr. If you want to we can try and fix it once everything else is working, otherwise it is on my list-of-things-I'll-eventually-do 😄

Manishearth · 2016-06-10T19:34:42Z

components/script/dom/url.rs

+            type_string: blob.Type().to_string(),
+            filename: None, // XXX: the filename is currently only in File object now
+            size: blob.Size(),
+            bytes: bytes.to_vec(),


Shoudlnt we be checking for file blobs? And slice blobs, but you haven't implemented that yet.

Yes, you are right. I haven't yet make Blob as powerful as it is expected to. But this field is necessary anyway.

izgzhen · 2016-06-11T00:20:38Z

For now just send it from the script thread and add a comment saying that net should actually be determining it from the pipeline id. Fetch/http_loader does this too right now. Its wrong, but I wouldn't bother fixing it for the purpose of this pr. If you want to we can try and fix it once everything else is working, otherwise it is on my list-of-things-I'll-eventually-do 😄

Cool. Do we have an issue or something to track this?

Manishearth · 2016-06-11T07:42:25Z

Good idea. #11722

Manishearth · 2016-06-11T07:44:00Z

Okay, for now send the origin from script (and leave a comment saying we shouldn't trust it linking to that issue). If you want, we could land this first and focus on file blob uris later, or fix it in this PR itself, either way works for me.

izgzhen · 2016-06-11T07:56:51Z

Okay, I still have several gaps to fill. I am okay with the idea of leave a comment if the Origin proposal needs some time. Anyway I will keep tracking it.

highfive · 2016-06-11T08:38:16Z

New code was committed to pull request.

izgzhen · 2016-06-11T08:42:35Z

components/script/dom/url.rs

+    // https://w3c.github.io/FileAPI/#dfn-createObjectURL
+    pub fn CreateObjectURL(global: GlobalRef, blob: &Blob) -> DOMString {
+        /// XXX: Second field is an unicode-serialized Origin, it is a temporary workaround
+        ///      and should not be trusted. See issue https://github.com/servo/servo/issues/11722


Noted both here and in BlobURLStoreMsg definition.

izgzhen · 2016-06-11T08:43:27Z

components/script/dom/url.rs

+
+    // https://w3c.github.io/FileAPI/#unicodeSerializationOfBlobURL
+    fn unicode_serialization_blob_url(origin: &str, id: &str) -> String {
+        // Step 1, 2


filled now. I feel like there is no more gaps to fill in this PR topic.

Manishearth · 2016-06-11T08:53:00Z

Do any new wpt tests pass? Do you know how to update expectations?

@bors-servo try

bors-servo · 2016-06-11T08:53:03Z

⌛ Trying commit 89c2aca with merge 6a19455...


        Auto merge of #11716 - izgzhen:impl-blob-url-dom, r=<try>

@Manishearth

Implement Blob URL's DOM interfaces r? @Manishearth Implement the two functions in `URL` to create/revoke Blob URLs, and related code to approximate our proposed design to make things work together.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix part of #10539, related to #11131  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11716)

highfive · 2016-06-11T09:18:39Z

New code was committed to pull request.

izgzhen · 2016-06-11T09:19:38Z

Do any new wpt tests pass? Do you know how to update expectations?

Yes, sorry about that I forgot to check this out. I have pushed the new code.

Manishearth · 2016-06-11T09:57:16Z

components/script/dom/url.rs

+
+            this method call does nothing. User agents may display a message on the error console.
+
+            XXX: First condition can't be enforced given current design


Why not?

Anyway, this step seems to already be run on the fm thread, sort of.

Change this comment to mention that the first step is unnecessary, since closed blobs do not exist in the store.

Manishearth · 2016-06-11T10:05:36Z

@bors-servo r+

bors-servo · 2016-06-11T10:05:37Z

📌 Commit 6672534 has been approved by Manishearth

Manishearth · 2016-06-11T10:13:24Z

looks like the manifest_changed thing failed.

Apply this diff, and r=manishearth

diff --git a/tests/wpt/metadata/MANIFEST.json b/tests/wpt/metadata/MANIFEST.json
index f7861cb..9689adf 100644
--- a/tests/wpt/metadata/MANIFEST.json
+++ b/tests/wpt/metadata/MANIFEST.json
@@ -36026,6 +36026,12 @@
     "deleted_reftests": {},
     "items": {
       "testharness": {
+        "FileAPI/url/url_xmlhttprequest.html": [
+          {
+            "path": "FileAPI/url/url_xmlhttprequest.html",
+            "url": "/FileAPI/url/url_xmlhttprequest.html"
+          }
+        ],
         "cssom-view/scrolling-no-browsing-context.html": [
           {
             "path": "cssom-view/scrolling-no-browsing-context.html",

@bors-servo r- delegate+

bors-servo · 2016-06-11T10:13:25Z

✌️ @izgzhen can now approve this pull request

izgzhen · 2016-06-11T10:36:53Z

Ahh my bad ... I thought both the ini and the metadata needs to be updated


        Auto merge of #11716 - izgzhen:impl-blob-url-dom, r=<try>

@Manishearth

Implement Blob URL's DOM interfaces r? @Manishearth Implement the two functions in `URL` to create/revoke Blob URLs, and related code to approximate our proposed design to make things work together.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix part of #10539, related to #11131  - [x] There are tests for these changes OR   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11716)

Manishearth · 2016-06-14T13:51:32Z

That happened because the blob: url for the worker source couldn't be loaded

oh, it tried to run a worker with a blob url. I didn't notice that 😄

bors-servo · 2016-06-14T14:12:57Z

💔 Test failed - linux-rel

highfive · 2016-06-14T14:13:03Z

  ▶ CRASH [expected OK] /html/semantics/embedded-content/the-iframe-element/change_parentage.html
  │ 
  │ Xlib:  extension &#34;XFree86-VidModeExtension&#34; missing on display &#34;:0&#34;.
  │ ERROR:constellation::constellation: Panic: ScriptThread: received an event message for a layout channel that is not associated with this script thread.This is a bug.
  │ ERROR:constellation::constellation: Backtrace:
  │ frame #0  - 0x00007f1e404c0eed - backtrace::backtrace::trace::hccde8df28b4db2a2
  │ frame #1  - 0x00007f1e404c0e75 - backtrace::capture::Backtrace::new::h42f95930bb8c5ee8
  │ frame #2  - 0x00007f1e3f4e4df9 - _&lt;F as alloc..boxed..FnBox&lt;A&gt;&gt;::call_box::h89adae1a802be550
  │ frame #3  - 0x00007f1e404b2728 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::he2b22674ad1748f3
  │ frame #4  - 0x00007f1e4078030c - std::panicking::rust_panic_with_hook::h983af77c1a2e581b
  │ frame #5  - 0x00007f1e4079a711 - std::panicking::begin_panic::he426e15a3766089a
  │ frame #6  - 0x00007f1e40781b7a - std::panicking::begin_panic_fmt::hdddb415186c241e7
  │ frame #7  - 0x00007f1e4079a6ae - rust_begin_unwind
  │ frame #8  - 0x00007f1e407d0caf - core::panicking::panic_fmt::hf4e16cb7f0d41a25
  │ frame #9  - 0x00007f1e407d8364 - core::option::expect_failed::hdb92832549f56a85
  │ frame #10 - 0x00007f1e3f4f1e31 - script::script_thread::ScriptThread::handle_msg_from_script::h47e979ae1e7fb676
  │ frame #11 - 0x00007f1e3f53dc3f - script::script_thread::ScriptThread::handle_msgs::_$u7b$$u7b$closure$u7d$$u7d$::hb50cf7fd97943b65
  │ frame #12 - 0x00007f1e3f5251e7 - script::script_thread::ScriptThread::handle_msgs::h1e1abab71191c950
  │ frame #13 - 0x00007f1e3f4e34e7 - std::panicking::try::call::h78dcd5319adf08fa
  │ frame #14 - 0x00007f1e407a490b - __rust_try
  │ frame #15 - 0x00007f1e407a48ae - __rust_maybe_catch_panic
  │ frame #16 - 0x00007f1e3f4e475d - _&lt;F as alloc..boxed..FnBox&lt;A&gt;&gt;::call_box::hce043844655ea6a9
  │ frame #17 - 0x00007f1e407988d4 - std::sys::thread::Thread::new::thread_start::h9c883b6d445ece46
  │ frame #18 - 0x00007f1e3c5d2183 - start_thread
  │ frame #19 - 0x00007f1e3c0e937c - clone
  │ frame #20 - 0x0000000000000000 - &lt;unknown&gt;
  │ 
  └ ERROR:constellation::constellation: Pipeline failed in hard-fail mode.  Crashing!

izgzhen · 2016-06-14T14:35:43Z

@jdm Before I retry, do I have to consider the "Worker::PostMessage panicked" problem?

bors-servo · 2016-06-14T15:23:17Z

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

jdm · 2016-06-14T16:55:40Z

Yes, I would prefer that.

izgzhen · 2016-06-15T10:25:01Z

@jdm I checked the code a bit and found that the worker constructor will load resource (load_whole_resource) from the Blob URL, which will finally go to here.

However, since I have not yet routed the blob-kind request to file manager thread, it is supposed to return NetworkError or something (which might contribute to the TIMEOUT of worker? not sure), rather than crash out of no clear reason ...

Besides, I can't reproduce this locally, and also I am not ready to route the blob URL request yet (still needs more integration), I personally prefer to check if this PR will break again on homu and maybe submit a new issue for it (if it is unreproducible).

bors-servo · 2016-06-17T00:56:20Z

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

jdm · 2016-06-17T01:11:26Z

@izgzhen It should be enough to use let _ = ... instead of the .unwrap() in Worker::PostMessage, since step 9 of https://html.spec.whatwg.org/multipage/comms.html#dom-messageport-postmessage indicates that a nonexistent communication channel should result in a silent error.

izgzhen · 2016-06-17T11:33:32Z

components/script/dom/worker.rs

+
+        // NOTE: step 9 of https://html.spec.whatwg.org/multipage/comms.html#dom-messageport-postmessage
+        // indicates that a nonexistent communication channel should result in a silent error.
+        let _ = self.sender.send((address, WorkerScriptMsg::DOMMessage(data)));


@jdm changed here.


        Implement Blob URL's DOM interfaces

jdm · 2016-06-17T12:25:28Z

@bors-servo: r=Manishearth

bors-servo · 2016-06-17T12:25:30Z

📌 Commit 4d33793 has been approved by Manishearth

bors-servo · 2016-06-17T12:26:17Z

⌛ Testing commit 4d33793 with merge 0c9d0eb...


        Auto merge of #11716 - izgzhen:impl-blob-url-dom, r=Manishearth

@Manishearth

Implement Blob URL's DOM interfaces r? @Manishearth Implement the two functions in `URL` to create/revoke Blob URLs, and related code to approximate our proposed design to make things work together.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix part of #10539, related to #11131  - [x] There are tests for these changes OR   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11716)

bors-servo · 2016-06-17T13:13:05Z

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

highfive assigned Manishearth Jun 10, 2016

highfive added the S-awaiting-review label Jun 10, 2016

Manishearth reviewed Jun 10, 2016
View changes

izgzhen force-pushed the izgzhen:impl-blob-url-dom branch from c501d00 to 89c2aca Jun 11, 2016

izgzhen reviewed Jun 11, 2016
View changes

izgzhen force-pushed the izgzhen:impl-blob-url-dom branch from 89c2aca to 6672534 Jun 11, 2016

Manishearth reviewed Jun 11, 2016
View changes

highfive added S-awaiting-merge and removed S-awaiting-review labels Jun 11, 2016

izgzhen force-pushed the izgzhen:impl-blob-url-dom branch from 6672534 to 4f08428 Jun 11, 2016

highfive added the S-tests-failed label Jun 14, 2016

highfive added the S-needs-rebase label Jun 14, 2016

izgzhen force-pushed the izgzhen:impl-blob-url-dom branch from e419f66 to 47248dc Jun 14, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Jun 14, 2016

highfive added the S-needs-rebase label Jun 17, 2016

izgzhen force-pushed the izgzhen:impl-blob-url-dom branch from 47248dc to f514a94 Jun 17, 2016

izgzhen reviewed Jun 17, 2016
View changes

Implement Blob URL's DOM interfaces

Loading status checks…

4d33793

izgzhen force-pushed the izgzhen:impl-blob-url-dom branch from f514a94 to 4d33793 Jun 17, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Jun 17, 2016

bors-servo merged commit 4d33793 into servo:master Jun 17, 2016
2 checks passed

2 checks passed

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Jun 17, 2016


		this method call does nothing. User agents may display a message on the error console.

		XXX: First condition can't be enforced given current design

servo / servo

Join GitHub today

Implement Blob URL's DOM interfaces #11716

Implement Blob URL's DOM interfaces #11716

Conversation

izgzhen commented Jun 10, 2016 • edited

highfive commented Jun 10, 2016

highfive commented Jun 10, 2016

izgzhen commented Jun 10, 2016

Manishearth commented Jun 10, 2016

This comment has been minimized.

Manishearth Jun 10, 2016

This comment has been minimized.

izgzhen Jun 11, 2016

izgzhen commented Jun 11, 2016

Manishearth commented Jun 11, 2016

Manishearth commented Jun 11, 2016

izgzhen commented Jun 11, 2016

highfive commented Jun 11, 2016

This comment has been minimized.

izgzhen Jun 11, 2016

This comment has been minimized.

izgzhen Jun 11, 2016

Manishearth commented Jun 11, 2016

bors-servo commented Jun 11, 2016

highfive commented Jun 11, 2016

izgzhen commented Jun 11, 2016 • edited

This comment has been minimized.

Manishearth Jun 11, 2016

This comment has been minimized.

Manishearth Jun 14, 2016

Manishearth commented Jun 11, 2016

bors-servo commented Jun 11, 2016

Manishearth commented Jun 11, 2016

bors-servo commented Jun 11, 2016

izgzhen commented Jun 11, 2016

Manishearth commented Jun 14, 2016

bors-servo commented Jun 14, 2016

highfive commented Jun 14, 2016

izgzhen commented Jun 14, 2016

bors-servo commented Jun 14, 2016

jdm commented Jun 14, 2016

izgzhen commented Jun 15, 2016

bors-servo commented Jun 17, 2016

jdm commented Jun 17, 2016

This comment has been minimized.

izgzhen Jun 17, 2016

jdm commented Jun 17, 2016

bors-servo commented Jun 17, 2016

bors-servo commented Jun 17, 2016

bors-servo commented Jun 17, 2016

izgzhen commented Jun 10, 2016 •

edited

izgzhen commented Jun 11, 2016 •

edited