Make network listener runnable cancellable

[cbrewster]

This keeps scripts from executing after its script thread has been shut down.

---

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

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

---

This change is

[highfive]

Heads up! This PR modifies the following files:

• @KiChjang: components/script/dom/htmllinkelement.rs, components/script/network_listener.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/htmlscriptelement.rs, components/script/dom/htmlmediaelement.rs, components/script/script_thread.rs

[highfive]

Warning

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

[jdm]

Here's a test that might work:

• parent.html adds an iframe loading child.html in a cross-origin manner (to ensure a separate script thread)
• child.html has a load event listener that appends an external script to the document
• this script uses wptserve's network filter to delay the script's network response by one second (foo.js?pipe=trickle(d1))
• parent.html's iframe load event removes the iframe, then finishes the test three seconds later

[cbrewster]

@jdm turns out, the panic only occurs if the iframe is same-origin, which seems bad since they could share the same script thread.

[cbrewster]

I added a test case that fails w/o the cancellable changes.

[cbrewster]

I want to run a few tries on this to make sure it fixes #11703.
@bors-servo try

[bors-servo]

⌛ Trying commit 0cd7688 with merge a4fb625...

[jdm]

If the test in question shuts down the shared script thread, that's a problem that we should be fixing :)

[cbrewster]

@jdm after the iframe is removed, the page still runs fine with the test results. So I assume that means the script thread for the parent isn't shut down.

[jdm]

True!

[jdm]

This script should call a function in the parent page which would make the test fail. Something like
window.script_executed = t.unreached_func("script executed in removed iframe") in the parent, and in the child: script_executed = parent.script_executed; then invoke script_executed() in this JS file.

[cbrewster]

@bors-servo try

[bors-servo]

⌛ Trying commit 411acaf with merge 7b83970...

[cbrewster]

Well this does not seem to fix the intermittent :(

[bors-servo]

💔 Test failed - linux-rel

[jdm]

The backtrace of the most recent panic is coming from a DOMManipulationTask runnable, not a network one.

[cbrewster]

@jdm is it possible to do something similar with that runnable?

[jdm]

What I'm wondering is why we're still running events for a script thread that's been shut down...

[jdm]

Oh, it's not the script thread that is shut down, it's a window that's had its JS context information cleared because it is not supposed to be needed any more. Bah.

[jdm]

There are a bunch of things in DOMManipulationTask that use Runnable which could theoretically be hooked the same way. The FireEvent and FireSimpleEvent messages would need special handling to ignore those messages if they're targeted at elements of a window that are closed, I guess ;(

[Ms2ger]

Fwiw, it's never been clear to me what the JSContext clearing is for.

[jdm]

The goal was to avoid hanging on to JS memory longer than necessary, since clearing it can end up removing the last user of the JS runtime.

[cbrewster]

I think I will leave this one as it is, and open a new PR when trying to solve the issue with DOMManipulationTask, so this should be ready for review.

[Ms2ger]

I don't think so; the script thread should keep hanging on to the runtime/context.

[Manishearth]

So the wrapper uses Relaxed atomic ordering. Which means that even if the bool is written to from the script thread, it's not necessary that the other thread will see this write in time.

We should either use SeqCst ordering, or Release on the script thread side writes and acquire from the wrapper reads.

Everything else

---

Reviewed 5 of 6 files at r1, 1 of 5 files at r3, 1 of 1 files at r4, 3 of 3 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/script-not-executed-after-shutdown-child.html, line 9 [r5] (raw file):

  s = document.createElement('script');
  s.type = 'text/javascript';
  s.src = 'script-not-executed-after-shutdown.js?pipe=trickle(d1)';

Perhaps a longer delay would ensure intermittents won't happen on a system experiencing heavy load (like our testing infrastructure at times)?

(Bump the 3000 up as well, of course)

---

Comments from Reviewable

[cbrewster]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/script-not-executed-after-shutdown-child.html, line 9 [r5] (raw file):

Previously, Manishearth (Manish Goregaokar) wrote…

Perhaps a longer delay would ensure intermittents won't happen on a system experiencing heavy load (like our testing infrastructure at times)?

(Bump the 3000 up as well, of course)

Done.

---

tests/wpt/web-platform-tests/html/semantics/scripting-1/the-script-element/script-not-executed-after-shutdown.js, line 1 [r4] (raw file):

Previously, jdm (Josh Matthews) wrote…

This script should call a function in the parent page which would make the test fail. Something like

window.script_executed = t.unreached_func("script executed in removed iframe") in the parent, and in the child: script_executed = parent.script_executed; then invoke script_executed() in this JS file.

Done.

---

Comments from Reviewable

[Manishearth]

@bors-servo r+

---

Reviewed 6 of 6 files at r6, 4 of 4 files at r7, 2 of 2 files at r8.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[bors-servo]

📌 Commit 5526756 has been approved by Manishearth

[bors-servo]

⌛ Testing commit 5526756 with merge b487a43...

[bors-servo]

💔 Test failed - linux-rel

[Manishearth]

@bors-servo retry

• Seemed to fail compile without any error? Wat.

[bors-servo]

⌛ Testing commit 5526756 with merge 12260b2...

[bors-servo]

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