Assert fail #18439 #23021

ejmg · 2019-03-13T16:39:52Z

This PR addresses #18439 by removing an assert statement that forces a panic whenever LoadBlocker is dropped during a GC sweep and receives a None SCRIPT_THREAD_ROOT value from mark_document_with_no_blocked_loads(). Instead of panicking on the assert, we remove it and let the None value pass silently.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #18439

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

This change is


        removed spurrious assert in Drop for LoadBlocker


        let mark_document_with_no_blocked_loads() take a None value silently

highfive · 2019-03-13T16:40:01Z

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

highfive · 2019-03-13T16:40:03Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/script_thread.rs, components/script/document_loader.rs
@KiChjang: components/script/script_thread.rs, components/script/document_loader.rs

highfive · 2019-03-13T16:40:08Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify script code, but no tests are modified. Please consider adding a test!

jdm · 2019-03-14T14:45:51Z

error: unused import: `std::thread`
  --> components/script/document_loader.rs:17:5
   |
17 | use std::thread;
   |     ^^^^^^^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`

jdm

This looks like what I expected to see :)

jdm · 2019-03-14T14:49:05Z

components/script/document_loader.rs

-        if !thread::panicking() {
-            debug_assert!(self.load.is_none());
+        if let Some(script_thread) = self.load.take() {
+            self.doc.finish_load(script_thread);


We should do Some(load) instead of Some(script_thread); the value inside the Option isn't related to the script thread.

jdm · 2019-03-14T14:49:05Z

components/script/script_thread.rs

+                    .docs_with_no_blocking_loads
+                    .borrow_mut()
+                    .insert(Dom::from_ref(doc));
+            }


Let's scope the unsafe block a little bit narrower:

SCRIPT_THREAD_ROOT.with(|root| { if let Some(script_thread) = root.get() { let script_thread = unsafe { &*script_thread }; script_thread .docs_with_no_blocking_loads .borrow_mut() .insert(Dom::from_ref(doc)); } });

thank you! I didn't know I could deref the bound value in an if let like that.

jdm · 2019-03-14T15:27:57Z

I've spent a bunch of time trying to write an automated test that tickles this problem, but I have not been able to do so.


        ran ./mach fmt

- removed unused import in document_loader.rs - limit unsafe block in mark_document_with_no_blocked_loads in script_thread.rs - changed name of if let value to load in Drop for LoadBlocker in document_loader.rs

ejmg · 2019-03-14T21:39:13Z

I've made the changes requested so far and rebased them such that there are only three commits, the first two are the actual changes required for the issue (one per file change) and the last one combines all requested changes from code review.

Let me know if this works and or if there are more changes desired 🙂

ejmg · 2019-03-18T17:18:35Z

testing a release build with my edits and I have been unable to replicate the crashes mentioned in #18439.

jdm · 2019-03-18T17:51:01Z

@bors-servo r+

bors-servo · 2019-03-18T17:51:02Z

📌 Commit be82d7c has been approved by jdm

bors-servo · 2019-03-18T17:51:10Z

⌛ Testing commit be82d7c with merge d1b3d13...


        Auto merge of #23021 - ejmg:assert_fail_#18439, r=jdm

Assert fail #18439  This PR addresses #18439 by removing an assert statement that forces a panic whenever `LoadBlocker` is dropped during a GC sweep and receives a `None` `SCRIPT_THREAD_ROOT` value from `mark_document_with_no_blocked_loads()`. Instead of panicking on the assert, we remove it and let the None value pass silently. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #18439  - [ ] 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/23021)

bors-servo · 2019-03-18T18:45:22Z

💔 Test failed - mac-rel-wpt4

jdm · 2019-03-18T18:46:48Z

@bors-servo retry

#22955

bors-servo · 2019-03-18T18:46:52Z

⌛ Testing commit be82d7c with merge fde786a...


        Auto merge of #23021 - ejmg:assert_fail_#18439, r=jdm

Assert fail #18439  This PR addresses #18439 by removing an assert statement that forces a panic whenever `LoadBlocker` is dropped during a GC sweep and receives a `None` `SCRIPT_THREAD_ROOT` value from `mark_document_with_no_blocked_loads()`. Instead of panicking on the assert, we remove it and let the None value pass silently. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #18439  - [ ] 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/23021)

bors-servo · 2019-03-18T19:42:13Z

💔 Test failed - mac-rel-css2

jdm · 2019-03-18T19:44:34Z

@bors-servo retry

#23054

bors-servo · 2019-03-18T19:44:38Z

⌛ Testing commit be82d7c with merge 0fac8f2...


        Auto merge of #23021 - ejmg:assert_fail_#18439, r=jdm

Assert fail #18439  This PR addresses #18439 by removing an assert statement that forces a panic whenever `LoadBlocker` is dropped during a GC sweep and receives a `None` `SCRIPT_THREAD_ROOT` value from `mark_document_with_no_blocked_loads()`. Instead of panicking on the assert, we remove it and let the None value pass silently. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #18439  - [ ] 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/23021)

bors-servo · 2019-03-18T21:22:52Z

☀️ Test successful - android-mac, arm32, arm64, linux-rel-css, linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, magicleap, status-taskcluster
Approved by: jdm
Pushing 0fac8f2 to master...

ejmg added 2 commits Mar 13, 2019

removed spurrious assert in Drop for LoadBlocker

d6b3eb6

let mark_document_with_no_blocked_loads() take a None value silently

c713feb

highfive assigned ferjm Mar 13, 2019

highfive added the S-awaiting-review label Mar 13, 2019

jdm requested changes Mar 14, 2019

View changes

jdm assigned jdm and unassigned ferjm Mar 14, 2019

jdm added S-needs-code-changes and removed S-awaiting-review labels Mar 14, 2019

ran ./mach fmt

Loading status checks…

be82d7c

- removed unused import in document_loader.rs - limit unsafe block in mark_document_with_no_blocked_loads in script_thread.rs - changed name of if let value to load in Drop for LoadBlocker in document_loader.rs

ejmg force-pushed the ejmg:assert_fail_#18439 branch from 321fdd8 to be82d7c Mar 14, 2019

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 14, 2019

ejmg mentioned this pull request Mar 18, 2019

Intermittent panic reflowing document from Document::finish_load (/html/browsers/windows/nested-browsing-contexts/name-attribute.window.html, /html/browsers/the-window-object/closed-attribute.window.html) #22955

Closed

highfive added S-awaiting-merge and removed S-awaiting-review labels Mar 18, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Mar 18, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Mar 18, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Mar 18, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Mar 18, 2019

highfive removed the S-awaiting-merge label Mar 18, 2019

servo / servo

Assert fail #18439 #23021

Assert fail #18439 #23021

ejmg commented Mar 13, 2019 •

edited by SimonSapin

highfive commented Mar 13, 2019

highfive commented Mar 13, 2019

highfive commented Mar 13, 2019

jdm commented Mar 14, 2019

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Mar 14, 2019

ejmg commented Mar 14, 2019

ejmg commented Mar 18, 2019

jdm commented Mar 18, 2019

bors-servo commented Mar 18, 2019

bors-servo commented Mar 18, 2019

bors-servo commented Mar 18, 2019

jdm commented Mar 18, 2019

bors-servo commented Mar 18, 2019

bors-servo commented Mar 18, 2019

jdm commented Mar 18, 2019

bors-servo commented Mar 18, 2019

bors-servo commented Mar 18, 2019

servo / servo

Join GitHub today

Assert fail #18439 #23021

Assert fail #18439 #23021

Conversation

ejmg commented Mar 13, 2019 • edited by SimonSapin

highfive commented Mar 13, 2019

highfive commented Mar 13, 2019

highfive commented Mar 13, 2019

jdm commented Mar 14, 2019

jdm left a comment

This comment has been minimized.

jdm Mar 14, 2019

This comment has been minimized.

jdm Mar 14, 2019

This comment has been minimized.

ejmg Mar 14, 2019

jdm commented Mar 14, 2019

ejmg commented Mar 14, 2019

ejmg commented Mar 18, 2019

jdm commented Mar 18, 2019

bors-servo commented Mar 18, 2019

bors-servo commented Mar 18, 2019

bors-servo commented Mar 18, 2019

jdm commented Mar 18, 2019

bors-servo commented Mar 18, 2019

bors-servo commented Mar 18, 2019

jdm commented Mar 18, 2019

bors-servo commented Mar 18, 2019

bors-servo commented Mar 18, 2019

ejmg commented Mar 13, 2019 •

edited by SimonSapin