Remove redundant windowproxy transplanting

[gterzian]

---

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors
• These changes fix #___ (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:

• @asajeffrey: components/script/script_thread.rs, components/script/dom/window.rs
• @KiChjang: components/script/script_thread.rs, components/script/dom/window.rs

[highfive]

Warning

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

[gterzian]

@asajeffrey The call to self.window_proxy().set_currently_active(self); inside window.resume seems redundant in two cases:

1. When a new page is loaded, and we create a new windowproxy. See

   servo/components/script/script_thread.rs

   Line 3270 in d7433c9

   window.resume();

2. When a new page is loaded, and we re-use a windowproxy, since we will already do the transplant at

   servo/components/script/script_thread.rs

   Line 3021 in d7433c9

   window_proxy.set_currently_active(&*window);

So I think the call is only necessary when we set the activity of a document to full-active, and there is no in progress load for that document(essentially I guess that's when re re-use a doc in the bfcache, and just set it to fully active).

In that case, the call is done inside document.set_activity, via a call to window.resume. I think it's probably clearer to do it as I've changed it here, and to remove the call inside window.resume, since most cases when we resume a window we do not need to transplant the windowproxy.

[asajeffrey]

This seems like a dangerous way to fix the issue, calling resume should make the document active. I'm concerned that with this change, we'd get weird race conditions about things like nested documents A with iframe B with iframe C, if C is resumed then B is resumed, since this should result in C being fully active but I'm not sure it will with this revision.

Is the case you're worried about one where we're transplanting a window proxy even though it's already in the right realm? If so, can we deal with this by adding a check to set_window to exit early if the proxy is already in the same realm as the window?

[gterzian]

Is the case you're worried about one where we're transplanting a window proxy even though it's already in the right realm?

What I've noticed is that when we load a new url in an existing browsing-context, we hit the branch at

servo/components/script/script_thread.rs

Line 3021 in c475755

window_proxy.set_currently_active(&*window);

Then later we hit it again at

servo/components/script/dom/window.rs

Line 1949 in c475755

self.window_proxy().set_currently_active(self);

So we simply do the transplant twice, as far as I can tell.

In the other cases, we do the transplant only once as part of window.resume, and I still wonder whether it's not redundant, since in those other cases we actually create a windowproxy using the window, as in WindowProxy::new(&window, ..). Do we then still need to do a set_window after that?

---

The only other place in the code I could find that uses window.resume is at

servo/components/script/dom/document.rs

Line 507 in c475755

self.window().resume();

,

which is called from

servo/components/script/script_thread.rs

Line 2410 in c475755

document.set_activity(activity);

That case seems necessary, since it deals with an existing window that becomes fully-active inside an existing browsing context.

So essentially that's the only place I added an explicit call to set_currently_active, as it seems necessary, as opposed to doing it in each call to resume, in which case it's often redundant.

---

can we deal with this by adding a check to set_window to exit early if the proxy is already in the same realm as the window?

Could we catch the redundancies inside

servo/components/script/dom/windowproxy.rs

Line 605 in c475755

pub fn set_currently_active(&self, window: &Window) {

by checking the pipeline id of the window against self.currently_active? Yes we could because in those cases they're already the same, and then I assume the call to set_window is redundant?

It might still be better to only call set_currently_active when it's necessary, as opposed to making it idempotent?

---

I'm concerned that with this change, we'd get weird race conditions about things like nested documents A with iframe B with iframe C, if C is resumed then B is resumed, since this should result in C being fully active but I'm not sure it will with this revision.

There might be something I'm missing, and from reading the code it seems the above mentioned cases provide an exhaustive list of when set_window is called, so if the redundancies are correctly identified it seems we should not get weird side-effects.

Iframes are dealt as part of

servo/components/script/script_thread.rs

Line 3024 in c475755

let iframe = parent_info.and_then(|parent_id| {

and it seems that they get their own newly created windowproxy each time, hence the subsequent call to set_window that happens as part of window.resume would seem redundant.

[asajeffrey]

I guess I'm worried that this change removes the transplanting from resume but not suspend, so they're not inverses. I see what you mean though, it is odd that local_window_proxy has a side-effect of setting the active document.

[gterzian]

Ok, should I simply check the pipeline of the window against the currently active one, and return early, inside

servo/components/script/dom/windowproxy.rs

Line 605 in c475755

pub fn set_currently_active(&self, window: &Window) {

That way we keep the current structure, and we just make "redundant" calls idempotent?

If self.currently_active is already the same as the pipeline id of the window passed as an argument, we never have to actually call set_window, or is there something else going on at the JS level?

[gterzian]

By the way, I was also surprised to see that a call to suspend always sets a DissimilarOriginWindow as the current window on the proxy...

See

servo/components/script/dom/windowproxy.rs

Line 613 in c475755

let window = DissimilarOriginWindow::new(&*globalscope, self);

[asajeffrey]

Yeah, that is odd. I think it's because if the replacement window is similar-origin, then the constellation will send another message activating it. But it does mean there's a transient state where it appears to be a dissimilar-origin window, possibly resulting in spurious security errors.

[gterzian]

Ok, the latest commit provides a fix that doesn't change the overall code stucture, ready for another review @asajeffrey

[asajeffrey]

A possible niggle about log levels, but otherwise LGTM.

[asajeffrey]

Make this a debug! rather than a warn!?

[gterzian]

Ok, done.

[asajeffrey]

@bors-servo r+

[bors-servo]

📌 Commit 9acf3f6 has been approved by asajeffrey

[bors-servo]

⌛ Testing commit 9acf3f6 with merge 91469aa...

[bors-servo]

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: asajeffrey
Pushing 91469aa to master...