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

iframe.goBack() doesn't paint the previous pipeline #9919

Closed
paulrouget opened this issue Mar 8, 2016 · 12 comments
Closed

iframe.goBack() doesn't paint the previous pipeline #9919

paulrouget opened this issue Mar 8, 2016 · 12 comments

Comments

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Mar 8, 2016

STR:

  • load test case (see "foo")
  • click on "go to next page" (see "bar")
  • click on goBack

"foo" should show up.

<!-- dom.mozbrowser.enabled needed -->
<iframe style="display:block;width:300px;height:300px" mozbrowser="true" src="data:,foo"></iframe>
<button onclick="f.src = 'data:,bar'">go to next page</button>
<button onclick="f.goBack()">goBack</button>
<button onclick="f.goForward()">goForward</button>
<script>
  var f = document.querySelector("iframe");
</script>
@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Mar 15, 2016

So first, I think we revoke paint permission of the wrong pipeline:

// Revoke paint permission from a pipeline, and all children.

Here we traverse the frame tree, but the top level frame of that tree should not use pipeline(frame.current) but pipeline(pipeline_id). Or, revoke_paint_permission should be called before the new pipeline is set to frame.current.

But that's apparently not enough.

The right frame tree is sent to the compositor. Paint permissions appear to be properly set.

With WR, the old page is not replace by the new one.
Without WR, the new page is replaced by a blank page.

@jdm
Copy link
Member

@jdm jdm commented Mar 15, 2016

cc @glennw

@glennw
Copy link
Member

@glennw glennw commented Mar 15, 2016

@paulrouget I'm not sure I understand what's wrong with that code. On the first iteration through that loop, pipeline_id should be equal to frame.current - is it not? From there it should loop through any iframes in the free tree from that root. The iframe goBack() code certainly used to work and I don't think that part has changed for quite some time, so I suspect a regression elsewhere.

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Mar 15, 2016

On the first iteration through that loop, pipeline_id should be equal to frame.current - is it not?

If I understand correctly how this works, it is not.

frame.current is set to next (which is correct), and then we call revoke_paint_permission(prev) (which is correct), but within revoke_paint_permission, we use frame.current.

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Mar 15, 2016

regression: c72d0c2

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Mar 15, 2016

@pcwalton any idea what might have gone wrong with c72d0c2?

Also - related (fixed) issue: #8081

@paulrouget paulrouget removed their assignment Mar 16, 2016
pcwalton added a commit to pcwalton/servo that referenced this issue Mar 24, 2016
navigation.

Since WebRender uses the pipeline ID stored in the iframe element to
determine which pipeline to display, it had better be kept up to date!

Closes servo#9919.
pcwalton added a commit to pcwalton/servo that referenced this issue Mar 24, 2016
navigation.

Since WebRender uses the pipeline ID stored in the iframe element to
determine which pipeline to display, it had better be kept up to date!

Closes servo#9919.
pcwalton added a commit to pcwalton/servo that referenced this issue Mar 24, 2016
navigation.

Since WebRender uses the pipeline ID stored in the iframe element to
determine which pipeline to display, it had better be kept up to date!

Closes servo#9919.
@pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Mar 24, 2016

@paulrouget Should be fixed by #10159.

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Mar 24, 2016

follow up for the revoke_paint_permission thing: #10162

pcwalton added a commit to pcwalton/servo that referenced this issue Mar 24, 2016
navigation.

Since WebRender uses the pipeline ID stored in the iframe element to
determine which pipeline to display, it had better be kept up to date!

Closes servo#9919.
@pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Mar 28, 2016

Can we close this in favor of #10162, in the absence of known actual browser.html problems this is causing?

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Mar 29, 2016

So this is fixed.

#10162 doesn't cause any bug afaict. Just thought I filed it just in case.

@paulrouget paulrouget closed this Mar 29, 2016
@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Mar 29, 2016

Sorry, not fixed, I though #10159 landed.

@paulrouget paulrouget reopened this Mar 29, 2016
@pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Mar 29, 2016

Oh right, sorry about that.

pcwalton added a commit to pcwalton/servo that referenced this issue Apr 1, 2016
navigation.

Since WebRender uses the pipeline ID stored in the iframe element to
determine which pipeline to display, it had better be kept up to date!

Closes servo#9919.
bors-servo added a commit that referenced this issue Apr 1, 2016
script: Make iframes know their pipeline IDs at all times, even after navigation.

Since WebRender uses the pipeline ID stored in the iframe element to
determine which pipeline to display, it had better be kept up to date!

Closes #9919.

r? @jdm

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10159)
<!-- Reviewable:end -->
pcwalton added a commit to pcwalton/servo that referenced this issue Apr 28, 2016
navigation.

Since WebRender uses the pipeline ID stored in the iframe element to
determine which pipeline to display, it had better be kept up to date!

Closes servo#9919.
bors-servo added a commit that referenced this issue Apr 28, 2016
script: Make iframes know their pipeline IDs at all times, even after navigation.

Since WebRender uses the pipeline ID stored in the iframe element to
determine which pipeline to display, it had better be kept up to date!

Closes #9919.

r? @jdm

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10159)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.