Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Window.open and related infrastructure #20678

Merged
merged 7 commits into from Aug 11, 2018

share event-loops based on eTLD+1

  • Loading branch information
gterzian committed Aug 10, 2018
commit e27ba16c3fda8a851b358b00852c2e0784649702
@@ -264,18 +264,13 @@ pub struct Constellation<Message, LTF, STF> {
/// WebRender thread.
webrender_api_sender: webrender_api::RenderApiSender,

/// The set of all event loops in the browser. We generate a new
/// event loop for each registered domain name (aka eTLD+1) in
/// each top-level browsing context. We store the event loops in a map
/// indexed by top-level browsing context id
/// (as a `TopLevelBrowsingContextId`) and registered
/// domain name (as a `Host`) to event loops. This double
/// indirection ensures that separate tabs do not share event
/// loops, even if the same domain is loaded in each.
/// The set of all event loops in the browser.
/// We store the event loops in a map
/// indexed by registered domain name (as a `Host`) to event loops.
/// It is important that scripts with the same eTLD+1
/// share an event loop, since they can use `document.domain`
/// to become same-origin, at which point they can share DOM objects.
event_loops: HashMap<TopLevelBrowsingContextId, HashMap<Host, Weak<EventLoop>>>,
event_loops: HashMap<Host, Weak<EventLoop>>,

This comment has been minimized.

Copy link
@nox

nox Jul 13, 2018

Member

I don't understand this change. What happens if 2 tabs are from the same domain name now?

This comment has been minimized.

Copy link
@gterzian

gterzian Jul 13, 2018

Author Member

With this change, if 2 tabs are from the same domain name, they will share an event-loop/script-thread. Previously, such sharing could only happen between same-domain browsing-contexts within the same tab.

This comment has been minimized.

Copy link
@nox

nox Jul 13, 2018

Member

But 2 tabs from the same domain name shouldn't share an event loop if they are unrelated, AFAIK.

This comment has been minimized.

Copy link
@gterzian

gterzian Jul 13, 2018

Author Member

From what I understand, @asajeffrey suggested to do it this way for now to support opener, and to revisit it later as part of process isolation. See #20678 (comment)

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Jul 14, 2018

Member

@nox the problem is that adding opener complicates the definition of Unit Of Related Similar-Origin Browsing Contexts. Before adding opener, BCs in different tabs were unrelated, but with it BCs are related if they can be reached by opener. Sigh.

This is all currently in flux, e.g. https://groups.google.com/a/chromium.org/forum/m/#!topic/isolation-policy/zueJF9ad20g so I suspect the easiest thing to do is revisit this issue as part of process isolation.

This comment has been minimized.

Copy link
@gterzian

gterzian Jul 15, 2018

Author Member

Thanks for the link, interesting discussion...

I've looked a bit into the spec, and would like to share the below findings and interpretations(which might be wrongly interpreted off course):

  1. It is very clear from the spec that a unit-of-related-similar-origin-browsing-contexts(who came up with that name!) must share an event-loop.
  2. I cannot find a suggestion that same-origin tabs, that would not actually be part of such a unit, as not allowed to share an event-loop.
  3. The difference between being part of such a unit or not, would be whether they can directly reach each other, for example via the opener. So running on the same event-loop will not make them directly reachable in itself(right?).
  4. Also, such two same-origin tabs that aren't reachable will still be familiar with each other, and if somehow they actually knew each other's name, they would be allowed to navigate each other, see Step 6 of #the-rules-for-choosing-a-browsing-context-given-a-browsing-context-name which states "there exists a browsing context whose name is the same as name, and current is familiar with that browsing context, and the user agent determines that the two browsing contexts are related enough that it is ok if they reach each other(...)". Also see in the big table above that the entry "name that exists with different top, if familiar and one permitted sandboxed navigator"(would be interesting to have a test for that and see how other implementations behave).

What I conclude, is that it might actually be ok to run two same-origin tabs on the same event-loop, and we could also let them navigate each other by name(however this is separate from having a direct reference to each other via opener, or the return value of window.open).

This comment has been minimized.

Copy link
@nox

nox Jul 16, 2018

Member

Opening two tabs with github.com, those two tabs should never share the same event loop.

A "unit of related similar-origin browsing contexts" is first and foremost a "unit of related browsing contexts", and 2 separate tabs are not a unit of related browsing contexts, because their browsing contexts aren't directly reachable from one another.

You are correct that they will be familiar, but I don't see why they would be allowed to navigate each other. At the very least this goes against what I've always had as a mental image of tabs and browsing contexts.

To me allowing unrelated tabs to share an event loop is too much of a Web regression to r+ that, so I'll defer to @jdm for the final decision.

This comment has been minimized.

Copy link
@gterzian

gterzian Jul 16, 2018

Author Member

Another option we have, is to revert to an earlier patch I made which would, when creating the openee, check for a matching host within the event-loops of the opener only. It might not cover all edge cases(what happens when the opener navigates to a different origin?), but it could be a start.

(On the navigation, I did some testing and indeed being 'familiar with' and knowing the name of the bc doesn't give you navigation, at least not in FF and Chrome)

This comment has been minimized.

Copy link
@gterzian

gterzian Jul 16, 2018

Author Member

Lastly, having come across a few more things in the spec makes me wonder again if sharing event-loops between same-origin tabs would indeed be a problem:

"There is also at most one event loop per unit of related similar-origin browsing contexts(though several units of related similar-origin browsing contexts can have a shared event loop)"(emphasis mine, taken from https://html.spec.whatwg.org/multipage/browsers.html#groupings-of-browsing-contexts)

This note seems to imply that even two tabs with different origins could share an event loop.

This sentence also points in the same direction: "There must be at least one browsing context event loop per user agent, and at most one per unit of related similar-origin browsing contexts."https://html.spec.whatwg.org/multipage/webappapis.html#event-loop.

The 'direction' this all seem to point to, is that it's more important to ensure that all browsing contexts inside a "unit of related similar-origin browsing contexts"(which includes an auxiliary and it's opener) share an event-loop, then it is to prevent unrelated browsing contexts from sharing one.

An extreme example: it would seem more spec-compliant to put all browsing contexts into one event-loop, as that would ensure one loop per "unit of related similar-origin browsing contexts", then to prevent unrelated tabs from sharing a loop, but to not have one loop per "unit of related similar-origin browsing contexts" as a result.

Could be a bug in the spec also... I couldn't find the commits for these two sentences so don't know what the thinking behind them was...

This comment has been minimized.

Copy link
@gterzian

gterzian Jul 17, 2018

Author Member

@jdm, in case you missed the mention above in the barrage of messages, we(me, @nox and @asajeffrey) would like to ask you to settle the above discussion regarding a policy to share event loops. All the info you need is probably found above, and then some, however here is a summary:

  1. Until this PR, Servo supported sharing an event-loop within a unit-of-related-similar-origin-browsing-contexts only if all browsing contexts within that unit were all descendants of the same top-level-browsing-context, or the top-level itself. A top-level-browsing-context would act as a boundary outside of which sharing an event-loop wasn't possible.
  2. This PR implements creating auxiliary browsing contexts, which are top-level-browsing-context that, if they have the same origin as their opener, would be part of the same "unit-of-related-similar-origin-browsing-contexts" as their opener, hence would need to share the same event-loop. This requires expanding the current policy, since an auxiliary will by definition be a 'new' top-level-browsing-context when created.
  3. @asajeffrey suggested we simply share event-loops based on same-origin, which is the current implementation in this PR. By definition this would make an opener/openee with the same origin share an event-loop. We would then revisit this sharing as part of upcoming work on process isolation.
  4. @nox has reservations about the fact that this new sharing policy would also allow two unrelated same-origin top-level-browsing-contexts(which wouldn't be part of the same "unit-of-related-similar-origin-browsing-contexts") to share an event-loop.
  5. From reading the spec, I am of the opinion that it would seem more important for spec compliance to have one event-loop per "unit-of-related-similar-origin-browsing-contexts", than it is to prevent sharing of an event-loop with unrelated browsing contexts. If my reading is correct, the current proposed change would appear like a net win, and it has a lot of simplicity going for it. We could later decide to go beyond the spec and do something more complicated to restrict sharing across tabs to openee/opener only, whereas by not going forward with the current change, we wouldn't have a spec compliant implementation of "unit-of-related-similar-origin-browsing-contexts".

This comment has been minimized.

Copy link
@jdm

jdm Jul 18, 2018

Member

I think the current implementation is fine, and we can prevent unrelated-but-same-origin browsing contexts from sharing event loops separately from this change. Please at least file an issue to keep track of that work.

This comment has been minimized.

Copy link
@gterzian

gterzian Jul 19, 2018

Author Member

Thanks for looking into this, I've filed #21206

This comment has been minimized.

Copy link
@nox

nox Jul 30, 2018

Member

@jdm Not sure what you mean, do you mean it's ok for now to have unrelated-but-same-origin browsing contexts share the same event loop?


joint_session_histories: HashMap<TopLevelBrowsingContextId, JointSessionHistory>,

@@ -718,9 +713,7 @@ where
None => (None, None),
Some(host) => {
let event_loop = self
.event_loops
.get(&top_level_browsing_context_id)
.and_then(|map| map.get(&host))
.event_loops.get(&host)
.and_then(|weak| weak.upgrade());
match event_loop {
None => (None, Some(host)),
@@ -806,9 +799,8 @@ where
"Adding new host entry {} for top-level browsing context {}.",
host, top_level_browsing_context_id
);
self.event_loops
.entry(top_level_browsing_context_id)
.or_insert_with(HashMap::new)
let _ = self
.event_loops
.insert(host, Rc::downgrade(&pipeline.event_loop));
}

@@ -1905,18 +1897,7 @@ where
Some(opener_pipeline) => (opener_pipeline, opener_pipeline.browsing_context_id),
None => return warn!("Auxiliary loaded url in closed pipeline {}.", opener_pipeline_id),
};
let opener_host = match reg_host(&opener_pipeline.url) {
Some(host) => host,
None => return warn!("Auxiliary loaded pipeline with no url {}.", opener_pipeline_id),
};
let script_sender = opener_pipeline.event_loop.clone();
// https://html.spec.whatwg.org/multipage/#unit-of-related-similar-origin-browsing-contexts
// If the auxiliary shares the host/scheme with the creator, they share an event-loop.
// So the first entry for the auxiliary, itself currently "about:blank",
// is the event-loop for the current host of the creator.
self.event_loops.entry(new_top_level_browsing_context_id)
.or_insert_with(HashMap::new)
.insert(opener_host, Rc::downgrade(&script_sender));
Pipeline::new(new_pipeline_id,
new_browsing_context_id,
new_top_level_browsing_context_id,
@@ -3565,10 +3546,6 @@ where
session_history.remove_entries_for_browsing_context(browsing_context_id);
}

if BrowsingContextId::from(browsing_context.top_level_id) == browsing_context_id {
self.event_loops.remove(&browsing_context.top_level_id);
}

let parent_info = self
.pipelines
.get(&browsing_context.pipeline_id)
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.