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
Remove redundant windowproxy transplanting #23967
Conversation
Heads up! This PR modifies the following files:
|
6a35726
to
b8d5cd2
Compare
@asajeffrey The call to
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 |
0951060
to
223fa8f
Compare
This seems like a dangerous way to fix the issue, calling 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 |
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
Then later we hit it again at servo/components/script/dom/window.rs Line 1949 in c475755
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 The only other place in the code I could find that uses servo/components/script/dom/document.rs Line 507 in c475755
which is called from servo/components/script/script_thread.rs Line 2410 in c475755
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
Could we catch the redundancies inside servo/components/script/dom/windowproxy.rs Line 605 in c475755
by checking the pipeline id of the window against It might still be better to only call
There might be something I'm missing, and from reading the code it seems the above mentioned cases provide an exhaustive list of when Iframes are dealt as part of servo/components/script/script_thread.rs Line 3024 in c475755
and it seems that they get their own newly created windowproxy each time, hence the subsequent call to |
I guess I'm worried that this change removes the transplanting from |
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
That way we keep the current structure, and we just make "redundant" calls idempotent? If |
By the way, I was also surprised to see that a call to See servo/components/script/dom/windowproxy.rs Line 613 in c475755
|
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. |
223fa8f
to
5c03337
Compare
5c03337
to
d6e1820
Compare
Ok, the latest commit provides a fix that doesn't change the overall code stucture, ready for another review @asajeffrey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A possible niggle about log levels, but otherwise LGTM.
components/script/dom/windowproxy.rs
Outdated
let dest_pipeline_id = globalscope.pipeline_id(); | ||
if let Some(pipeline_id) = self.currently_active() { | ||
if pipeline_id == dest_pipeline_id { | ||
return warn!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this a debug!
rather than a warn!
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done.
d6e1820
to
9acf3f6
Compare
@bors-servo r+ |
📌 Commit 9acf3f6 has been approved by |
Remove redundant windowproxy transplanting <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23967) <!-- Reviewable:end -->
☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster |
./mach build -d
does not report any errors./mach test-tidy
does not report any errorsThis change is