Improve implementation and add testing regarding file manager thread #11552

izgzhen · 2016-06-02T07:36:13Z

First there is a more completed unit test. And in the test running, I found a runtime error Serde(Custom("bincode does not support Deserializer::deserialize)) when reading response from file manage thread. I analyzed a bit and found that it is probably caused by the Uuid field. I temporarily work around it by making the Id essentially a string wrapped inside SelectedFileId.

Related to PR #11131.

./mach build -d does not report any errors
./mach test-tidy does not report any errors

There are tests for these changes

This change is

highfive · 2016-06-02T07:36:17Z

Heads up! This PR modifies the following files:

@KiChjang: components/net/filemanager_thread.rs, components/net_traits/filemanager_thread.rs, components/net_traits/filemanager_thread.rs, components/net_traits/lib.rs, components/net_traits/lib.rs, components/script/dom/bindings/trace.rs, components/net_traits/Cargo.toml, components/net_traits/Cargo.toml, components/script/dom/blob.rs

bors-servo · 2016-06-02T12:07:48Z

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

emilio · 2016-06-02T23:52:13Z

tests/unit/net/filemanager_thread.rs

@@ -27,7 +82,7 @@ fn test_filemanager() {
        let _ = chan.send(FileManagerThreadMsg::SelectFile(tx));

        match rx.try_recv() {


nit: Probably simpler to just assert!(rx.try_recv().is_err(), "xxx")

Cool, thanks

izgzhen · 2016-06-03T10:33:15Z

r? @Manishearth

Not intended to bug you, just thinking it might be easier to track whatever decision I am making :P

izgzhen · 2016-06-08T13:28:02Z

@Manishearth I just updated this. You might want to have a look if you got time :)

Manishearth · 2016-06-09T14:02:51Z

components/net/filemanager_thread.rs

-                FileManagerThreadMsg::ReadFile(sender, id) => self.read_file(sender, id),
+                FileManagerThreadMsg::ReadFile(sender, id) => {
+                    match self.try_read_file(id) {
+                        Ok(buffer) => sender.send(Ok(buffer)).expect("FileManager send error"),


Not expect, we should silently ignore the sending error.

send() fails if the receiver doesn't exist. This can happen if a content thread crashed, in this case. Content threads crashing should not bring down the chrome process.

(Basically, the chrome process should never crash unless it's an irrecoverable error for the entire browser)

highfive · 2016-06-09T14:37:38Z

New code was committed to pull request.

highfive · 2016-06-09T14:40:17Z

New code was committed to pull request.

Manishearth · 2016-06-09T17:50:16Z

components/net/filemanager_thread.rs

-                FileManagerThreadMsg::ReadFile(sender, id) => self.read_file(sender, id),
+                FileManagerThreadMsg::ReadFile(sender, id) => {
+                    match self.try_read_file(id) {
+                        Ok(buffer) => sender.send(Ok(buffer)).unwrap(),


Unwrap also panics. let _ = xyz.send();

Manishearth · 2016-06-09T17:53:46Z

tests/unit/net/filemanager_thread.rs

-            Err(FileManagerThreadError::InvalidSelection) => {},
-            _ => assert!(false, "Should be an invalid selection before dialog is implemented"),
+                if let Ok(vec) = msg {
+                    assert!(test_file_content == vec, "Read content differs");


This can be condensed using expect, but not necessary

Manishearth · 2016-06-09T17:55:25Z

tests/unit/net/filemanager_thread.rs

+    if let Ok(mut handler) = opened {
+        handler.read_to_end(&mut test_file_content)
+               .expect("Read tests/unit/net/test.txt error");
+    }


We should just panic here via expect if opened is not OK, and get rid of the opened_ok checks later

s/later/below

Well, I made too loose an assumption before that the test.txt might not exist at all 😂


        Add unit testing to file manager; Replace uuid by stringified version…

… in FileManagerThreadMsg

highfive · 2016-06-09T18:21:06Z

New code was committed to pull request.

izgzhen · 2016-06-09T18:22:16Z

tests/unit/net/filemanager_thread.rs


 #[test]
 fn test_filemanager() {
    let chan: IpcSender<FileManagerThreadMsg> = FileManagerThreadFactory::new();

+    // Try to open a dummy file "tests/unit/net/test.txt" in tree
+    let mut handler = File::open("test.txt").expect("test.txt is stolen");


@Manishearth Now it will panic if test.txt is missing

izgzhen · 2016-06-09T18:23:43Z

components/net/filemanager_thread.rs

+                FileManagerThreadMsg::ReadFile(sender, id) => {
+                    match self.try_read_file(id) {
+                        Ok(buffer) => { let _ = sender.send(Ok(buffer)); }
+                        Err(_) => { let _ = sender.send(Err(FileManagerThreadError::ReadFileError)); }


@Manishearth Now send should not crash file manger thread

Manishearth · 2016-06-09T18:58:04Z

@bors-servo r+

bors-servo · 2016-06-09T18:58:05Z

📌 Commit f8fa9aa has been approved by Manishearth

bors-servo · 2016-06-09T19:29:24Z

⌛ Testing commit f8fa9aa with merge 57c9dd8...


        Auto merge of #11552 - izgzhen:add-testing-fix-filemanager, r=Manishe…

…arth Improve implementation and add testing regarding file manager thread First there is a more completed unit test. And in the test running, I found a runtime error `Serde(Custom("bincode does not support Deserializer::deserialize))` when reading response from file manage thread. I analyzed a bit and found that it is probably caused by the `Uuid` field. I temporarily work around it by making the `Id` essentially a string wrapped inside `SelectedFileId`. Related to PR #11131.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11552)

bors-servo · 2016-06-09T20:11:05Z

💔 Test failed - mac-rel-wpt

highfive · 2016-06-09T20:11:09Z

  ▶ CRASH [expected OK] /dom/nodes/Node-parentNode.html
  │ 
  │ ERROR:constellation::constellation: Panic: resize sent to nonexistent pipeline
  │ ERROR:constellation::constellation: Backtrace:
  │ frame #0  - 0x000000010ad95e7e - backtrace::backtrace::trace::hccde8df28b4db2a2
  │ frame #1  - 0x000000010ad95e01 - backtrace::capture::Backtrace::new::h42f95930bb8c5ee8
  │ frame #2  - 0x000000010a2d2dc6 - _&lt;F as alloc..boxed..FnBox&lt;A&gt;&gt;::call_box::h6bf1f1518fe13d1a
  │ frame #3  - 0x000000010ad7b6f7 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::ha2bd86c312dc8d7a
  │ frame #4  - 0x000000010af8a5c4 - std::panicking::rust_panic_with_hook::h983af77c1a2e581b
  │ frame #5  - 0x0000000109c915e4 - std::panicking::begin_panic::h0bf39f6d43ab9349
  │ frame #6  - 0x000000010a332880 - script::script_thread::ScriptThread::handle_msgs::_$u7b$$u7b$closure$u7d$$u7d$::h41a556e2c2c94fe6
  │ frame #7  - 0x000000010a315372 - script::script_thread::ScriptThread::handle_msgs::h6a699373a020da16
  │ frame #8  - 0x000000010a2d1597 - std::panicking::try::call::hb8565a24f25031fc
  │ frame #9  - 0x000000010afa60db - __rust_try
  │ frame #10 - 0x000000010afa6075 - __rust_maybe_catch_panic
  │ frame #11 - 0x000000010a2d27f4 - _&lt;F as alloc..boxed..FnBox&lt;A&gt;&gt;::call_box::h78c73797281bb868
  │ frame #12 - 0x000000010afa1b58 - std::sys::thread::Thread::new::thread_start::h9c883b6d445ece46
  │ frame #13 - 0x00007fff9534f059 - _pthread_body
  │ frame #14 - 0x00007fff9534efd6 - _pthread_start
  │ 
  └ ERROR:constellation::constellation: Pipeline failed in hard-fail mode.  Crashing!

jdm · 2016-06-09T21:14:20Z

@bors-servo: retry

#11613

bors-servo · 2016-06-09T21:45:29Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, windows are reusable. Rebuilding only mac-rel-wpt...

bors-servo · 2016-06-09T22:27:18Z

💔 Test failed - mac-rel-wpt

highfive · 2016-06-09T22:27:21Z

  ▶ CRASH [expected OK] /dom/nodes/Node-parentNode.html
  │ 
  │ ERROR:constellation::constellation: Panic: resize sent to nonexistent pipeline
  │ ERROR:constellation::constellation: Backtrace:
  │ frame #0  - 0x00000001024c7dbe - backtrace::backtrace::trace::hccde8df28b4db2a2
  │ frame #1  - 0x00000001024c7d41 - backtrace::capture::Backtrace::new::h42f95930bb8c5ee8
  │ frame #2  - 0x0000000101a04d06 - _&lt;F as alloc..boxed..FnBox&lt;A&gt;&gt;::call_box::h6bf1f1518fe13d1a
  │ frame #3  - 0x00000001024ad637 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::ha2bd86c312dc8d7a
  │ frame #4  - 0x00000001026bc504 - std::panicking::rust_panic_with_hook::h983af77c1a2e581b
  │ frame #5  - 0x00000001013c3524 - std::panicking::begin_panic::h0bf39f6d43ab9349
  │ frame #6  - 0x0000000101a647c0 - script::script_thread::ScriptThread::handle_msgs::_$u7b$$u7b$closure$u7d$$u7d$::h41a556e2c2c94fe6
  │ frame #7  - 0x0000000101a472b2 - script::script_thread::ScriptThread::handle_msgs::h6a699373a020da16
  │ frame #8  - 0x0000000101a034d7 - std::panicking::try::call::hb8565a24f25031fc
  │ frame #9  - 0x00000001026d801b - __rust_try
  │ frame #10 - 0x00000001026d7fb5 - __rust_maybe_catch_panic
  │ frame #11 - 0x0000000101a04734 - _&lt;F as alloc..boxed..FnBox&lt;A&gt;&gt;::call_box::h78c73797281bb868
  │ frame #12 - 0x00000001026d3a98 - std::sys::thread::Thread::new::thread_start::h9c883b6d445ece46
  │ frame #13 - 0x00007fff9534f059 - _pthread_body
  │ frame #14 - 0x00007fff9534efd6 - _pthread_start
  │ 
  └ ERROR:constellation::constellation: Pipeline failed in hard-fail mode.  Crashing!

  ▶ CRASH [expected OK] /html/semantics/embedded-content/the-iframe-element/change_parentage.html
  │ 
  │ ERROR:constellation::constellation: Panic: resize sent to nonexistent pipeline
  │ ERROR:constellation::constellation: Backtrace:
  │ frame #0  - 0x0000000107988dbe - backtrace::backtrace::trace::hccde8df28b4db2a2
  │ frame #1  - 0x0000000107988d41 - backtrace::capture::Backtrace::new::h42f95930bb8c5ee8
  │ frame #2  - 0x0000000106ec5d06 - _&lt;F as alloc..boxed..FnBox&lt;A&gt;&gt;::call_box::h6bf1f1518fe13d1a
  │ frame #3  - 0x000000010796e637 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::ha2bd86c312dc8d7a
  │ frame #4  - 0x0000000107b7d504 - std::panicking::rust_panic_with_hook::h983af77c1a2e581b
  │ frame #5  - 0x0000000106884524 - std::panicking::begin_panic::h0bf39f6d43ab9349
  │ frame #6  - 0x0000000106f257c0 - script::script_thread::ScriptThread::handle_msgs::_$u7b$$u7b$closure$u7d$$u7d$::h41a556e2c2c94fe6
  │ frame #7  - 0x0000000106f082b2 - script::script_thread::ScriptThread::handle_msgs::h6a699373a020da16
  │ frame #8  - 0x0000000106ec44d7 - std::panicking::try::call::hb8565a24f25031fc
  │ frame #9  - 0x0000000107b9901b - __rust_try
  │ frame #10 - 0x0000000107b98fb5 - __rust_maybe_catch_panic
  │ frame #11 - 0x0000000106ec5734 - _&lt;F as alloc..boxed..FnBox&lt;A&gt;&gt;::call_box::h78c73797281bb868
  │ frame #12 - 0x0000000107b94a98 - std::sys::thread::Thread::new::thread_start::h9c883b6d445ece46
  │ frame #13 - 0x00007fff9534f059 - _pthread_body
  │ frame #14 - 0x00007fff9534efd6 - _pthread_start
  │ 
  └ ERROR:constellation::constellation: Pipeline failed in hard-fail mode.  Crashing!

jdm · 2016-06-09T22:42:39Z

@bors-servo: retry

#11693 #11613


        Auto merge of #11552 - izgzhen:add-testing-fix-filemanager, r=Manishe…

…arth Improve implementation and add testing regarding file manager thread First there is a more completed unit test. And in the test running, I found a runtime error `Serde(Custom("bincode does not support Deserializer::deserialize))` when reading response from file manage thread. I analyzed a bit and found that it is probably caused by the `Uuid` field. I temporarily work around it by making the `Id` essentially a string wrapped inside `SelectedFileId`. Related to PR #11131.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11552)

bors-servo · 2016-06-09T23:14:31Z

⌛ Testing commit f8fa9aa with merge 6f9016c...

bors-servo · 2016-06-10T00:01:42Z

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

highfive assigned glennw Jun 2, 2016

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

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

emilio reviewed Jun 2, 2016
View changes

izgzhen force-pushed the izgzhen:add-testing-fix-filemanager branch from 977d445 to 2a53fdc Jun 3, 2016

highfive assigned Manishearth and unassigned glennw Jun 3, 2016

izgzhen force-pushed the izgzhen:add-testing-fix-filemanager branch 2 times, most recently from 4761a04 to 04b0703 Jun 3, 2016

jdm removed the S-needs-rebase label Jun 8, 2016

Manishearth reviewed Jun 9, 2016
View changes

izgzhen force-pushed the izgzhen:add-testing-fix-filemanager branch from 04b0703 to 4f22598 Jun 9, 2016

izgzhen force-pushed the izgzhen:add-testing-fix-filemanager branch from 4f22598 to 49b1243 Jun 9, 2016

Manishearth reviewed Jun 9, 2016
View changes

Add unit testing to file manager; Replace uuid by stringified version…

Loading status checks…

f8fa9aa

… in FileManagerThreadMsg

izgzhen force-pushed the izgzhen:add-testing-fix-filemanager branch from 49b1243 to f8fa9aa Jun 9, 2016

izgzhen reviewed Jun 9, 2016
View changes

highfive removed the S-awaiting-review label Jun 9, 2016

highfive added the S-awaiting-merge label Jun 9, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jun 9, 2016

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

highfive added S-awaiting-merge and removed S-tests-failed labels Jun 9, 2016

bors-servo merged commit f8fa9aa into servo:master Jun 10, 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 10, 2016

servo / servo

Improve implementation and add testing regarding file manager thread #11552

Improve implementation and add testing regarding file manager thread #11552

izgzhen commented Jun 2, 2016 •

edited

highfive commented Jun 2, 2016

bors-servo commented Jun 2, 2016

This comment has been minimized.

This comment has been minimized.

izgzhen commented Jun 3, 2016

izgzhen commented Jun 8, 2016

This comment has been minimized.

highfive commented Jun 9, 2016

highfive commented Jun 9, 2016

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.

highfive commented Jun 9, 2016

This comment has been minimized.

This comment has been minimized.

Manishearth commented Jun 9, 2016

bors-servo commented Jun 9, 2016

bors-servo commented Jun 9, 2016

bors-servo commented Jun 9, 2016

highfive commented Jun 9, 2016

jdm commented Jun 9, 2016

bors-servo commented Jun 9, 2016

bors-servo commented Jun 9, 2016

highfive commented Jun 9, 2016

jdm commented Jun 9, 2016

bors-servo commented Jun 9, 2016

bors-servo commented Jun 10, 2016

		@@ -27,7 +82,7 @@ fn test_filemanager() {
		let _ = chan.send(FileManagerThreadMsg::SelectFile(tx));

		match rx.try_recv() {

servo / servo

Join GitHub today

Improve implementation and add testing regarding file manager thread #11552

Improve implementation and add testing regarding file manager thread #11552

Conversation

izgzhen commented Jun 2, 2016 • edited

highfive commented Jun 2, 2016

bors-servo commented Jun 2, 2016

This comment has been minimized.

This comment has been minimized.

izgzhen commented Jun 3, 2016

izgzhen commented Jun 8, 2016

This comment has been minimized.

highfive commented Jun 9, 2016

highfive commented Jun 9, 2016

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.

highfive commented Jun 9, 2016

This comment has been minimized.

izgzhen Jun 9, 2016 • edited

This comment has been minimized.

izgzhen Jun 9, 2016 • edited

Manishearth commented Jun 9, 2016

bors-servo commented Jun 9, 2016

bors-servo commented Jun 9, 2016

bors-servo commented Jun 9, 2016

highfive commented Jun 9, 2016

jdm commented Jun 9, 2016

bors-servo commented Jun 9, 2016

bors-servo commented Jun 9, 2016

highfive commented Jun 9, 2016

jdm commented Jun 9, 2016

bors-servo commented Jun 9, 2016

bors-servo commented Jun 10, 2016

izgzhen commented Jun 2, 2016 •

edited

izgzhen Jun 9, 2016 •

edited

izgzhen Jun 9, 2016 •

edited