(Do not merge) Use the same script thread if new location is same origin

[KiChjang]

Fixes #5239.

---

This change is

[highfive]

Heads up! This PR modifies the following files:

• @asajeffrey: components/constellation/constellation.rs

[KiChjang]

@bors-servo try

[bors-servo]

⌛ Trying commit 4efad0f with merge 887ea49...

[bors-servo]

💔 Test failed - linux-rel

[jdm]

A couple comments:

• this is a change that I've intended to make for years
• it would be best to share the logic for "should we reuse this script thread" with the other related code in handle_script_loaded_url_in_iframe
• my biggest concern about this change has always been how to deal with it in the frame tree in the newly-shared script thread

To elaborate on the third point, consider what happens if the user clicks a same-origin link in a top-level browsing context. Currently script threads have a notion of a single root browsing context that gets used in lots of places (ScriptThread::root_browsing_context). This change would need to include modifications so that we can retain inactive root contexts and replace them with a new active one that is designated the real root.

[asajeffrey]

One thing we should bear in mind is that at some point we'll support document.domain, and we may need to think about what happens in the case that either the new or old pipeline had set its domain. Not a problem right now, as we don't support domain, but it my be something to think about.

[jdm]

What we've talked about in the past is always sharing script threads between any origins that could become same-origin through the use of document.domain.

[nox]

-S-awaiting-review +S-awaiting-answer

---

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

components/constellation/constellation.rs, line 1414 [r1] (raw file):

                    //                HTMLIFrameElement::contentDocument.
                    let script_chan = if source.url.host() == load_data.url.host() &&
                                         source.url.port() == load_data.url.port() {

Why can't you use UrlHelper::SameOrigin here?

---

Comments from Reviewable

[KiChjang]

@nox Because it's the wrong function, we should be comparing with the origin of the document, not the document URL's origin.

[nox]

@KiChjang There is a commenting issue then. That code doesn't use UrlHelper::SameOrigin and states:

FIXME(#10968): this should probably match the origin check in HTMLIFrameElement::contentDocument.

But then HTMLIFrameELement::contentDocument does use the helper and states:

FIXME(#10964): this should use the Document's origin and the origin of the incumbent settings object.

[KiChjang]

@Ms2ger wrote the comment. What do you think is a better wording?

[nox]

@bors-servo try

[KiChjang]

@nox There's a try run up there. I haven't touched this PR for a while.

[nox]

@KiChjang The logs disappeared.

[KiChjang]

@bors-servo try- clean retry

[KiChjang]

@bors-servo try- clean try

[KiChjang]

Ugh, I'll post a new commit soon.

[KiChjang]

@bors-servo try

[bors-servo]

⌛ Trying commit 31607cc with merge f646a65...

[bors-servo]

💔 Test failed - linux-rel

[KiChjang]

Ran 6841 tests finished in 746.0 seconds.
  • 6831 ran as expected. 1996 tests skipped.
  • 3 tests unexpectedly okay
  • 9 tests had unexpected subtest results

Tests with unexpected results:
  ▶ OK [expected TIMEOUT] /html/browsers/browsing-the-web/navigating-across-documents/012.html

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/navigating-across-documents/012.html:
  │ TIMEOUT [expected PASS] Link with onclick navigation to javascript url with delayed document.write and href navigation 
  └   → Test timed out

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/navigating-across-documents/005.html:
  │ TIMEOUT [expected PASS] Link with onclick navigation and href navigation 
  └   → Test timed out

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/navigating-across-documents/007.html:
  │ TIMEOUT [expected PASS] Link with onclick javascript url and href navigation 
  └   → Test timed out

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/navigating-across-documents/009.html:
  │ TIMEOUT [expected PASS] Link with onclick form submit to javascript url with document.write and href navigation 
  └   → Test timed out

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/navigating-across-documents/006.html:
  │ TIMEOUT [expected PASS] Link with onclick form submit and href navigation 
  └   → Test timed out

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/navigating-across-documents/010.html:
  │ TIMEOUT [expected PASS] Link with onclick form submit to javascript url with delayed document.write and href navigation 
  └   → Test timed out

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/navigating-across-documents/008.html:
  │ TIMEOUT [expected PASS] Link with onclick form submit to javascript url and href navigation 
  └   → Test timed out

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/navigating-across-documents/011.html:
  │ TIMEOUT [expected PASS] Link with onclick navigation to javascript url with document.write and href navigation 
  └   → Test timed out

  ▶ OK [expected TIMEOUT] /html/browsers/windows/browsing-context-names/001.html

  ▶ OK [expected TIMEOUT] /html/browsers/windows/browsing-context-names/002.html

  ▶ Unexpected subtest result in /html/browsers/windows/browsing-context-names/002.html:
  │ FAIL [expected PASS] Link with target=_blank, no rel
  │   → assert_unreached: Failed to get callback from opened window Reached unreachable code
  │ 
  │ @http://web-platform.test:8000/html/browsers/windows/browsing-context-names/002.html:10:18
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1402:20
  │ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1426:20
  └ step_timeout/<@http://web-platform.test:8000/resources/testharness.js:669:13

Ignore the CSS error, it's a known intermittent.

[KiChjang]

My thoughts on this would be to make the browsing_context field a DOMRefCell<Vec<BrowsingContext>>. This would be similar to the url_list field in Request. I'm just not sure if that's how we should handle things, as it could get quite messy with the browsing history stuff.

[asajeffrey]

A conversation on IRC: http://logs.glob.uno/?c=mozilla%23servo&s=7+Sep+2016&e=7+Sep+2016#c518850

tl;dr: the test should be if the new child is same-origin with it's parent, not same-origin with the old child.

[KiChjang]

I'm confused. What does child and parent mean in this context?

[bors-servo]

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

[asajeffrey]

See #633. We should share a script thread between any two pipelines with a) the same tab, and b) the same eTLD+1. Here, "same tab" means "same top-level ancestor", i.e. "same mozbrowser-or-root ancestor".

[KiChjang]

This is most probably blocked by #5322.