Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upneed to navigate back three times to return back one page #18564
Assignees
Comments
|
cc @asajeffrey |
|
This was found in servoshell, not sure if it's specific to that. |
|
Interesting. This might actually be doing the Spec Compatible Thing depending on how the youtube iframe is behaving: if it's navigating itself without enabling replacement, then it's meant to add a session history entry each time. Of course this isn't what the user (or content author) might be expecting :/ |
|
Firefox and Chrome both do the thing I expect (immediate navigation to home page), but they also support the video, so maybe this is just an issue in the fallback path. However, the next navigation after I get back always crashes Servo. See #18563 |
|
Yes, this looks like a bug with how we're handling the session history, there should only be one entry created, since there's just the iframe's initial about:blank then the embedded video. cc @cbrewster |
|
TMI: |
|
Hmm I see three It could be that youtube's JS has a fallback path where it creates extra iframes, which is causing the excess session history entries? |
|
@cpearce Is there a way to disable gecko's media stack so that YouTube embeds would behave more like current Servo to test the fallback path? |
|
Looking at the JS in FF's debugger, there are some suspicious var a=document.createElement("IFRAME");
a.style.display="none";
a.src="";
document.documentElement.appendChild(a);In Servo's impl of iframes this may well create more session history entries than FF does, since the iframe is being attached with We may be hitting whatwg/html#490 here. |
|
@jdm: this may be more concrete evidence of web content providers (in this case youtube!) relying on a particular semantics of initial about:blank handling. |
|
I believe this is #14720 |
|
@asajeffrey in your snippet from the embedded player, the The iframe will have |
|
@cbrewster the reason why the src attribute matters is that if it wasn't set then the clause from https://html.spec.whatwg.org/multipage/iframe-embed-object.html#process-the-iframe-attributes would apply:
|
|
Hmm: Shouldn't that be: self.navigate_or_reload_child_browsing_context(Some(load_data), NavigationType::Regular, true); |
|
Ah, replacing |
|
IRC chat: http://logs.glob.uno/?c=mozilla%23servo&s=20+Sep+2017&e=20+Sep+2017#c755990 TL;DR: we should be OK doing: asajeffrey@5fa2dfe: let replace = mode == ProcessingMode::FirstTime;
self.navigate_or_reload_child_browsing_context(Some(load_data), NavigationType::Regular, replace); |
bors-servo
added a commit
that referenced
this issue
Sep 21, 2017
…about-blank, r=<try> Script iframe intial load replaces about blank <!-- Please describe your changes on the following line: --> When navigating from the initial about:blank, do it with replacement enabled. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #18564 - [X] These changes do not require tests because the existing tests do the job <!-- 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/18587) <!-- Reviewable:end -->
bors-servo
added a commit
that referenced
this issue
Sep 21, 2017
…about-blank, r=<try> Script iframe intial load replaces about blank <!-- Please describe your changes on the following line: --> When navigating from the initial about:blank, do it with replacement enabled. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #18564 - [X] These changes do not require tests because the existing tests do the job <!-- 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/18587) <!-- Reviewable:end -->
bors-servo
added a commit
that referenced
this issue
Sep 21, 2017
…about-blank, r=<try> Script iframe intial load replaces about blank <!-- Please describe your changes on the following line: --> When navigating from the initial about:blank, do it with replacement enabled. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #18564 - [X] These changes do not require tests because the existing tests do the job <!-- 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/18587) <!-- Reviewable:end -->
|
I am not able to reproduce this issue in the latest nightly build using Alt + left arrow to go back. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
STR: