Delegate resource reading to embedder #20533

paulrouget · 2018-04-04T14:01:17Z

Now the embedder provides the content of the files itself. Now, on Android, we can use regular assets instead of unzipping all the resources on the scared at startup.

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

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

This change is

highfive · 2018-04-04T14:01:22Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/userscripts.rs, components/script/dom/servoparser/mod.rs, components/script/lib.rs, components/constellation/sandboxing.rs, components/script/Cargo.toml
@cbrewster: components/constellation/sandboxing.rs
@fitzgen: components/script/dom/userscripts.rs, components/script/dom/servoparser/mod.rs, components/script/lib.rs, components/script/Cargo.toml
@KiChjang: components/net/lib.rs, components/net/image_cache.rs, components/script/dom/userscripts.rs, components/script/dom/servoparser/mod.rs, components/net/connector.rs and 10 more

paulrouget · 2018-04-11T08:41:21Z

Note to self:

need to test if we broke sandboxing (most likely)
ml post about the change in arguments (--shaders and --userscripts)

paulrouget · 2018-04-16T08:23:32Z

eb91969 fixes the sandboxing issues.

paulrouget · 2018-04-16T11:06:48Z

Tested on Android and Mac. Sandboxing has been tested too.

paulrouget · 2018-04-16T11:12:55Z

@mbrubeck r?

paulrouget · 2018-04-17T09:32:39Z

(fixed the Windows failure)

mbrubeck · 2018-04-18T19:05:19Z

ports/servo/resources.rs

+            buffer
+        }
+        fn sandbox_access_files_rec(&self) -> Vec<PathBuf> {
+            vec![resources_dir_path().expect("Can't find resources directory")]


If this just returns paths to top-level directories (rather than recursively iterating over the files inside those directories), maybe it should be renamed to sandox_access_files_dirs.

mbrubeck · 2018-04-18T19:09:59Z

components/layout_thread/lib.rs

@@ -1740,11 +1741,11 @@ fn get_root_flow_background_color(flow: &mut Flow) -> webrender_api::ColorF {
 fn get_ua_stylesheets() -> Result<UserAgentStylesheets, &'static str> {
    fn parse_ua_stylesheet(
        shared_lock: &SharedRwLock,
-        filename: &'static str,
+        filename: &str,
+        content: String,


This could take Vec<u8> or &[u8] instead, and then the callers could use read_bytes instead of read_string to avoid needless UTF-8 validation.

mbrubeck

Looks good overall. Some minor suggestions/concerns below.

mbrubeck · 2018-04-18T22:38:03Z

ports/servo/resources.rs

+            path.push(file);
+            let mut buffer = vec![];
+            File::open(path).expect(&format!("Can't find file: {}", file))
+                .read_to_end(&mut buffer).expect("Can't read file");


This could use the new std::fs::read which pre-allocates a buffer.

mbrubeck · 2018-04-18T22:38:03Z

components/net/resource_thread.rs

+            let mut txt = String::new();
+            let mut file = File::open(PathBuf::from(path))
+                .expect("Couldn't not find certificate file");
+            file.read_to_string(&mut txt).expect("Cant read certificate");


This can use fs::read_to_string.

mbrubeck · 2018-04-18T22:38:03Z

components/net/connector.rs

-    ssl_connector_builder.set_ca_file(ca_file).expect("could not set CA file");
+    loop {
+        if let Some(index) = certs.rfind("-----BEGIN CERTIFICATE-----") {
+            let cert = certs.split_off(index);


We could avoid allocating and copying here, by keeping cert as &str instead of String, and doing:

let (cert, rest) = certs.split_at(i); certs = rest;

mbrubeck · 2018-04-18T22:38:03Z

components/net/connector.rs

+                }
+            }).expect("could not set CA file");
+        } else {
+            break;


It looks like this might skip the last certificate in the file, because it's not followed by a BEGIN CERTIFICATE line.

I use END CERTIFICATE now.

mbrubeck · 2018-04-18T22:38:03Z

components/net/connector.rs

    let mut ssl_connector_builder = SslConnectorBuilder::new(SslMethod::tls()).unwrap();
-    ssl_connector_builder.set_ca_file(ca_file).expect("could not set CA file");


Maybe we should add a function to the openssl crate that takes the certificate file contents as a string. Then we could continue to let OpenSSL do the parsing here.

It already has a method to parse the whole certs file, but it's a "all or non" operation (not like the previous method we were passing a file instead of a buffer). If it fails to load one of the certificate, they are all rejected. And in our case, it might fail because some certificates were already registered (so it's fine to ignore the error). So if we were to improve the Rust OpenSSL API, we should allow OpenSSL to ignore the duplicate error.

I can file a bug for that. Sounds to me that it's a OpenSSL issue, not a Rust OpenSSL problem though.

paulrouget · 2018-04-21T13:10:04Z

(comments addressed)

mbrubeck · 2018-04-21T15:28:15Z

@bors-servo r+

bors-servo · 2018-04-21T15:28:16Z

📌 Commit eced418 has been approved by mbrubeck

bors-servo · 2018-04-21T15:28:20Z

⌛ Testing commit eced418 with merge 7ced433...


        Auto merge of #20533 - paulrouget:res, r=mbrubeck

Delegate resource reading to embedder Now the embedder provides the content of the files itself. Now, on Android, we can use regular assets instead of unzipping all the resources on the scared at startup. ---  - [x] `./mach build -d` does not report any errors - [ ] `./mach build-geckolib` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #15635 (github issue number if applicable).  - [ ] 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="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20533)

bors-servo · 2018-04-21T15:52:17Z

💔 Test failed - linux-dev

bors-servo · 2018-04-27T08:33:20Z

⌛ Testing commit 9fb5795 with merge c28d6f0...


        Auto merge of #20533 - paulrouget:res, r=mbrubeck

Delegate resource reading to embedder Now the embedder provides the content of the files itself. Now, on Android, we can use regular assets instead of unzipping all the resources on the scared at startup. ---  - [x] `./mach build -d` does not report any errors - [ ] `./mach build-geckolib` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #15635 (github issue number if applicable).  - [ ] 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="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20533)

bors-servo · 2018-04-27T08:55:04Z

💔 Test failed - mac-rel-css1

paulrouget · 2018-04-27T09:51:34Z

@bors-servo retry

Infra: Servers failed to start.

bors-servo · 2018-04-27T11:12:47Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-wpt, mac-dev-unit, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt3, windows-msvc-dev are reusable. Rebuilding only linux-rel-css, mac-rel-css1, mac-rel-wpt2, mac-rel-wpt4...

nox · 2018-04-27T14:43:28Z

@bors-servo retry force

bors-servo · 2018-04-27T14:43:32Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt3, windows-msvc-dev are reusable. Rebuilding only mac-rel-wpt2, mac-rel-wpt4...

bors-servo · 2018-04-27T14:46:27Z

⛄ The build was interrupted to prioritize another pull request.

nox · 2018-04-27T14:47:45Z

Sorry again @paulrouget, Homu was sitting on its hands doing nothing.

@bors-servo p=1

bors-servo · 2018-04-27T15:14:07Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt3, windows-msvc-dev are reusable. Rebuilding only mac-rel-wpt2, mac-rel-wpt4...

bors-servo · 2018-04-27T15:34:17Z

💔 Test failed - mac-rel-wpt2

nox · 2018-04-27T15:37:17Z

@paulrouget CI is still a bit foobar'd. Please don't retry this, I'll do it myself once I know it can build without constant attention.

nox · 2018-04-27T17:50:13Z

@bors-servo r=mbrubeck

bors-servo · 2018-04-27T17:50:13Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

bors-servo · 2018-04-27T17:50:14Z

📌 Commit 9fb5795 has been approved by mbrubeck

bors-servo · 2018-04-27T17:50:20Z

⌛ Testing commit 9fb5795 with merge d1378d6...


        Auto merge of #20533 - paulrouget:res, r=mbrubeck

Delegate resource reading to embedder Now the embedder provides the content of the files itself. Now, on Android, we can use regular assets instead of unzipping all the resources on the scared at startup. ---  - [x] `./mach build -d` does not report any errors - [ ] `./mach build-geckolib` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #15635 (github issue number if applicable).  - [ ] 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="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20533)

bors-servo · 2018-04-27T19:14:48Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: mbrubeck
Pushing d1378d6 to master...

highfive assigned mbrubeck Apr 4, 2018

highfive added the S-awaiting-review label Apr 4, 2018

paulrouget force-pushed the paulrouget:res branch 5 times, most recently from 1cc07ad to 48f2222 Apr 4, 2018

paulrouget changed the title ~~[WIP] Delegate resource reading to embedder~~ Delegate resource reading to embedder Apr 11, 2018

paulrouget force-pushed the paulrouget:res branch from eb91969 to fca81e5 Apr 16, 2018

paulrouget force-pushed the paulrouget:res branch from fca81e5 to 9d15d6d Apr 16, 2018

paulrouget force-pushed the paulrouget:res branch from 9d15d6d to 481f5ba Apr 17, 2018

paulrouget requested a review from mbrubeck Apr 18, 2018

mbrubeck reviewed Apr 18, 2018

View changes

mbrubeck requested changes Apr 18, 2018

View changes

paulrouget force-pushed the paulrouget:res branch from 481f5ba to eced418 Apr 20, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Apr 21, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Apr 27, 2018

highfive added the S-tests-failed label Apr 27, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Apr 27, 2018

bors-servo mentioned this pull request Apr 27, 2018

refactor(net): removes direct ui invocation from filemgr thread #20480

Merged

3 of 5 tasks complete

bors-servo merged commit 9fb5795 into servo:master Apr 27, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Apr 27, 2018

This was referenced Apr 27, 2018

Dedupe syn, quote and unicode-xid #20649

Merged

Updating performance implementation and putting more measurements in #20459

Merged

emilio mentioned this pull request Apr 29, 2018

Unit tests that read prefs can randomly panic. #20710

Closed

paulrouget mentioned this pull request Sep 21, 2018

Enable WebAudio on Android #21730

Merged

3 of 3 tasks complete

		let mut ssl_connector_builder = SslConnectorBuilder::new(SslMethod::tls()).unwrap();
		ssl_connector_builder.set_ca_file(ca_file).expect("could not set CA file");

servo / servo

Join GitHub today

Delegate resource reading to embedder #20533

Delegate resource reading to embedder #20533

Conversation

paulrouget commented Apr 4, 2018 • edited

highfive commented Apr 4, 2018

paulrouget commented Apr 11, 2018

paulrouget commented Apr 16, 2018 • edited

paulrouget commented Apr 16, 2018

paulrouget commented Apr 16, 2018

paulrouget commented Apr 17, 2018

This comment has been minimized.

This comment has been minimized.

mbrubeck left a comment

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.

paulrouget commented Apr 21, 2018

mbrubeck commented Apr 21, 2018

bors-servo commented Apr 21, 2018

bors-servo commented Apr 21, 2018

bors-servo commented Apr 21, 2018

bors-servo commented Apr 27, 2018

bors-servo commented Apr 27, 2018

paulrouget commented Apr 27, 2018

bors-servo commented Apr 27, 2018

nox commented Apr 27, 2018

bors-servo commented Apr 27, 2018

bors-servo commented Apr 27, 2018

nox commented Apr 27, 2018

bors-servo commented Apr 27, 2018

bors-servo commented Apr 27, 2018

nox commented Apr 27, 2018

nox commented Apr 27, 2018

bors-servo commented Apr 27, 2018

bors-servo commented Apr 27, 2018

bors-servo commented Apr 27, 2018

bors-servo commented Apr 27, 2018

paulrouget commented Apr 4, 2018 •

edited

paulrouget commented Apr 16, 2018 •

edited