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 flickering #7868

Closed
paulrouget opened this issue Oct 5, 2015 · 26 comments
Closed

iframe flickering #7868

paulrouget opened this issue Oct 5, 2015 · 26 comments

Comments

@paulrouget
Copy link
Contributor

In browser.html, moving the mouse outside of the iframe makes the iframe flicker a lot.

This minimal test case shows some flickering. Load the page, then move the mouse over the red element (not the iframe). The iframe flickers once in a while. It's not as obvious as in browser.html, but still easy to see.

See these logs. Passed the initial load, the only thing that happens is the mouse hovering some text outside of the iframe. But I see layout/reflow operations, for both documents. Not sure if that suppose to happen. The flickering happens somewhere in the 2 last seconds.

Edit: Adding a requestAnimationFrame loop makes the flickering obvious. I've updated the example.

<iframe src='http://example.com'></iframe>

<script>
  var s = () => requestAnimationFrame(s);
  s();
</script>
@paulrouget
Copy link
Contributor Author

In some other logs, I see a lot of:

DEBUG:script::dom::window: script: performing reflow for goal ForDisplay reason WindowResize

…when there's not resize.

@paulrouget
Copy link
Contributor Author

Adding a requestAnimationFrame loop makes the flickering obvious. I've updated the example.

@paulrouget paulrouget changed the title iframe: lot of flickering when moving mouse iframe flickering and high CPU usage Oct 7, 2015
@paulrouget
Copy link
Contributor Author

From the logs, I see:

DEBUG:compositing::compositor: compositor: compositing
DEBUG:layout::layout_task: Done building display list.
DEBUG:layout::layout_task: Layout done!
DEBUG:script::dom::window: root has no dirty descendants; avoiding reflow (reason RequestAnimationFrame)
DEBUG:compositing::constellation: constellation got frame size message
DEBUG:script::dom::window: script: performing reflow for goal ForDisplay reason WindowResize

That looks suspicious to me. Where is the code that checks that the iframe frame did indeed change?

@paulrouget
Copy link
Contributor Author

Trying to figuring out what's going on. Struggling between the logs, lldb and some random println, I'm slowly starting to have a better picture of the situation.

Compositing ticks animations (layout_task.rs#tick_animations), which calls self.perform_post_style_recalc_layout_passes. Somewhere down this road, once the layers have been calculated (are they?), the size of the iframe is updated (update_subpage_size_if_necessary), the iframe document is then rebuilt (see log: "performing reflow for goal ForDisplay reason WindowResize").

I guess this is what makes the page flicker.

So somewhere in there we should have figured that the iframe size hasn't change, and never send the FrameSize message. Or maybe inside the iframe, we should have known that the new size is equal to the old one, and then not reflow.

The code I use to debug this:

<style>

  div {
    width: 100px;
    height: 100px;
    background: red;
  }

  iframe {
  }

</style>

<div style=""></div>

<iframe src='http://example.com'></iframe>

<script>
  document.documentElement.addEventListener('keyup', () => {
    requestAnimationFrame(() => {});
  });
</script>

@paulrouget
Copy link
Contributor Author

If anyone could tell me if I'm on the right track and give me a hint to where to look next, that would be very nice :)

@jdm
Copy link
Member

jdm commented Oct 8, 2015

If we want to short-circuit resize messages, the place to do so would be either ScriptTask::handle_resize (where we store the new size data), or ScriptTask::handle_resize_event (where we process the stored size data).

@jdm
Copy link
Member

jdm commented Oct 8, 2015

On the other hand, perhaps we could short-circuit in Constellation::handle_frame_size_msg instead when assigning to pipeline.size.

@pcwalton
Copy link
Contributor

pcwalton commented Oct 8, 2015

This could be a straightforward incremental reflow "jumpiness" bug and not iframe-specific, BTW.

@paulrouget paulrouget changed the title iframe flickering and high CPU usage iframe flickering Oct 9, 2015
@paulrouget
Copy link
Contributor Author

Not to self: when RAF is called, the compositor gets a InitializeLayersForPipeline message. Then, only if there's an iframe present, create_or_update_descendant_layer is called, which will down the road set a ResizeEvent into the iframe window, which will then trigger a force_reflow. So short circuiting in ScriptTask::handle_resize_event might work, but then the iframe will get a DOM resize event. So it needs happen earlier.

@paulrouget
Copy link
Contributor Author

Like @jdm said, the 2 options are ScriptTask::handle_resize and Constellation::handle_frame_size_msg.

@paulrouget
Copy link
Contributor Author

In ScriptTask::handle_resize, I only set the resize event if the new size is different from window's size. On load, the iframe is properly drawn, but then, when RAF is called, the content of the iframe disappear.

@paulrouget
Copy link
Contributor Author

short circuiting in ScriptTask::handle_resize_event might work, but then the iframe will get a DOM resize event

I was wrong. In there, we don't have to send the DOM event.

@paulrouget
Copy link
Contributor Author

the content of the iframe disappear

The display list doesn't include the iframe content when the force_reflow doesn't happen on the iframe.

This is a trace of what happens between the requestAnimationFrame and the force_reflow:

compositor.rs:
       change_running_animations_state()
       tick_animations_for_pipeline()

ConstellationMsg::TickAnimation
LayoutControlMsg::TickAnimations
Msg::TickAnimations

layout_task.rs:
      tick_animations()
      perform_post_style_recalc_layout_passes()
      perform_post_main_layout_passes()
      compute_abs_pos_and_build_display_list()
      // layout done

LayoutToPaintMsg::PaintInit

paint_task.rs:
      initialize_layers()

compositor_task.rs:
      initialize_layers_for_pipeline()

Msg::InitializeLayersForPipeline

compositor.rs:
      create_or_update_descendant_layer
      update_subpage_size_if_necessary

ConstellationMsg::FrameSize

constellation.rs:
      handle_frame_size_msg

ConstellationControlMsg::Resize

script_task.rs:
      handle_resize() // window.r().set_resize_event()
      handle_resize_event() // window.r().force_reflow();

Display list when the reflow happens:

perform_post_style_recalc_layout_passes
reflow_entire_document
compute_abs_pos_and_build_display_list
┌ DisplayList
│  ├─ StackingContext at Rect(800px×608px at (0px,0px)) with overflow Rect(800px×608px at (0px,0px)):
│  │  ├─ Block Backgrounds and Borders
│  │  │  ├─ SolidColor rgba(0, 0, 0, 0) @ Rect(800px×608px at (0px,0px)) (11b56f1c0)
│  │  │  ├─ SolidColor rgba(0, 0, 0, 0) @ Rect(784px×258.1166666666667px at (8px,8px)) (11b56f2b0)
│  │  │  └─ SolidColor rgba(1, 0, 0, 1) @ Rect(100px×100px at (8px,8px)) (11b56f2e0)
│  │  ├─ Layers
│  │  │  ├─ Layered StackingContext at Rect(304px×154px at (8px,108px)) with overflow Rect(304px×154px at (0px,0px)):
│  │  │  │  ├─ Content
│  │  │  │  │  ├─ SolidColor rgba(0, 0, 0, 0) @ Rect(304px×154px at (0px,0px)) (11b56f340)
│  │  │  │  │  └─ Border @ Rect(304px×154px at (0px,0px)) (11b56f340)

perform_post_style_recalc_layout_passes
reflow_entire_document
compute_abs_pos_and_build_display_list
┌ DisplayList
│  ├─ StackingContext at Rect(800px×608px at (0px,0px)) with overflow Rect(800px×608px at (0px,0px)):
│  │  ├─ Block Backgrounds and Borders
│  │  │  ├─ SolidColor rgba(0, 0, 0, 0) @ Rect(800px×608px at (0px,0px)) (11b56f1c0)
│  │  │  ├─ SolidColor rgba(0, 0, 0, 0) @ Rect(784px×258.1166666666667px at (8px,8px)) (11b56f2b0)
│  │  │  └─ SolidColor rgba(1, 0, 0, 1) @ Rect(100px×100px at (8px,8px)) (11b56f2e0)
│  │  ├─ Layers
│  │  │  ├─ Layered StackingContext at Rect(304px×154px at (8px,108px)) with overflow Rect(304px×154px at (0px,0px)):
│  │  │  │  ├─ Content
│  │  │  │  │  ├─ SolidColor rgba(0, 0, 0, 0) @ Rect(304px×154px at (0px,0px)) (11b56f340)
│  │  │  │  │  └─ Border @ Rect(304px×154px at (0px,0px)) (11b56f340)

perform_post_style_recalc_layout_passes
reflow_entire_document
compute_abs_pos_and_build_display_list
┌ DisplayList
│  ├─ StackingContext at Rect(304px×170px at (0px,0px)) with overflow Rect(304px×170px at (0px,0px)):
│  │  ├─ Block Backgrounds and Borders
│  │  │  ├─ SolidColor rgba(0, 0, 0, 0) @ Rect(304px×170px at (0px,0px)) (11d16f1c0)
│  │  │  ├─ SolidColor rgba(0, 0, 0, 0) @ Rect(288px×18.983333333333334px at (8px,16px)) (11d16f220)
│  │  │  ├─ SolidColor rgba(0, 0, 0, 0) @ Rect(288px×18.983333333333334px at (8px,16px)) (11d16f250)
│  │  │  └─ SolidColor rgba(0, 0, 0, 0) @ Rect(288px×18.983333333333334px at (8px,16px)) (11d16f280)
│  │  ├─ Content
│  │  │  └─ Text @ Rect(9.616666666666667px×15.983333333333333px at (8px,17.5px)) (11d16f2b0)

perform_post_style_recalc_layout_passes
reflow_entire_document
compute_abs_pos_and_build_display_list
┌ DisplayList
│  ├─ StackingContext at Rect(304px×170px at (0px,0px)) with overflow Rect(304px×170px at (0px,0px)):
│  │  ├─ Block Backgrounds and Borders
│  │  │  ├─ SolidColor rgba(0, 0, 0, 0) @ Rect(304px×170px at (0px,0px)) (11d16f1c0)
│  │  │  ├─ SolidColor rgba(0, 0, 0, 0) @ Rect(288px×18.983333333333334px at (8px,16px)) (11d16f220)
│  │  │  ├─ SolidColor rgba(0, 0, 0, 0) @ Rect(288px×18.983333333333334px at (8px,16px)) (11d16f250)
│  │  │  └─ SolidColor rgba(0, 0, 0, 0) @ Rect(288px×18.983333333333334px at (8px,16px)) (11d16f280)
│  │  ├─ Content
│  │  │  └─ Text @ Rect(9.616666666666667px×15.983333333333333px at (8px,17.5px)) (11d16f2b0)

Display list when the reflow in the iframe is skipped because its size hasn't changed:

perform_post_style_recalc_layout_passes
reflow_entire_document
compute_abs_pos_and_build_display_list
┌ DisplayList
│  ├─ StackingContext at Rect(800px×608px at (0px,0px)) with overflow Rect(800px×608px at (0px,0px)):
│  │  ├─ Block Backgrounds and Borders
│  │  │  ├─ SolidColor rgba(0, 0, 0, 0) @ Rect(800px×608px at (0px,0px)) (11df6f1c0)
│  │  │  ├─ SolidColor rgba(0, 0, 0, 0) @ Rect(784px×258.1166666666667px at (8px,8px)) (11df6f2b0)
│  │  │  └─ SolidColor rgba(1, 0, 0, 1) @ Rect(100px×100px at (8px,8px)) (11df6f2e0)
│  │  ├─ Layers
│  │  │  ├─ Layered StackingContext at Rect(304px×154px at (8px,108px)) with overflow Rect(304px×154px at (0px,0px)):
│  │  │  │  ├─ Content
│  │  │  │  │  ├─ SolidColor rgba(0, 0, 0, 0) @ Rect(304px×154px at (0px,0px)) (11df6f340)
│  │  │  │  │  └─ Border @ Rect(304px×154px at (0px,0px)) (11df6f340)

perform_post_style_recalc_layout_passes
reflow_entire_document
compute_abs_pos_and_build_display_list
┌ DisplayList
│  ├─ StackingContext at Rect(800px×608px at (0px,0px)) with overflow Rect(800px×608px at (0px,0px)):
│  │  ├─ Block Backgrounds and Borders
│  │  │  ├─ SolidColor rgba(0, 0, 0, 0) @ Rect(800px×608px at (0px,0px)) (11df6f1c0)
│  │  │  ├─ SolidColor rgba(0, 0, 0, 0) @ Rect(784px×258.1166666666667px at (8px,8px)) (11df6f2b0)
│  │  │  └─ SolidColor rgba(1, 0, 0, 1) @ Rect(100px×100px at (8px,8px)) (11df6f2e0)
│  │  ├─ Layers
│  │  │  ├─ Layered StackingContext at Rect(304px×154px at (8px,108px)) with overflow Rect(304px×154px at (0px,0px)):
│  │  │  │  ├─ Content
│  │  │  │  │  ├─ SolidColor rgba(0, 0, 0, 0) @ Rect(304px×154px at (0px,0px)) (11df6f340)
│  │  │  │  │  └─ Border @ Rect(304px×154px at (0px,0px)) (11df6f340)

@paulrouget
Copy link
Contributor Author

Trying to summarize the situation:

On requestAnimationFrame, the iframe flickers. On RAF, the iframe content is cleared (white), then the content is painted again. It stays white for a very short time.

The iframe document is forced to reflow on requestAnimationFrame.

This forced reflow should not happen. And that's probably related to the flickering (the cause or a sign that something is not behaving the way it should).

So I tried to prevent this forced reflow:

master...paulrouget:flickeringIframe

But then, after a requestAnimationFrame, the iframe content is cleared and it stays blank.

Dumping the display lists (see previous comment), I see that the one for the iframe is not present.

My guess is that the reflow of the iframe document was building a new display list, and because this reflow is not happening anymore, the display list is just … empty? Not built? Is there some DLBI happening that would eventually clear the iframe? What would be the right way to re-use the old display list?

There are some other things I don't understand here:

  • aren't we doing too many things on RAF? I see a full repaint with paint-flashing.
  • Looking at the logs in the previous comment, are the display lists built twice? If so, is that normal?
  • what the life time of a display list? or a layer?

Any help would be appreciated :)

@jdm
Copy link
Member

jdm commented Oct 12, 2015

Perhaps @glennw can answer some of these questions :)

@glennw
Copy link
Member

glennw commented Oct 14, 2015

Sorry, meant to look at this today and got distracted - will follow up soon.

@glennw
Copy link
Member

glennw commented Oct 14, 2015

It does seem like we're doing a paint after rAF even when not necessary. It looks like it updates the styles for any animations, and assumes that it will need to do a new display list generation and paint. But in the case of a rAF with no actual animations or node changes, this isn't the case. @pcwalton Is it easy to detect this case (no animations changed) and avoid the unnecessary paint?

A display list is built for each layout, and then handed off to the paint task. The paint task replaces the existing display list each time it receives a new display list from the layout task.

@paulrouget What happens if you early out from update_subpage_size_if_necessary() if the frame size is the same as the existing frame size? I haven't looked into it too deeply, but this seems like the right place to avoid spurious iframe reflows.

@paulrouget
Copy link
Contributor Author

But in the case of a rAF with no actual animations or node changes, this isn't the case. @pcwalton Is it easy to detect this case (no animations changed) and avoid the unnecessary paint?

Is the problem that the animation state is either AnimationsPresent, AnimationCallbacksPresent, NoAnimationsPresent or NoAnimationCallbacksPresent, and there's no way to know if a callback is present and no animation is running?

@paulrouget What happens if you early out from update_subpage_size_if_necessary() if the frame size is the same as the existing frame size? I haven't looked into it too deeply, but this seems like the right place to avoid spurious iframe reflows.

I don't know if it's possible to actually do it in update_subpage_size_if_necessary() (how would I get the old size of the iframe?), but if I do it a bit later (in ScriptTask::handle_resize_event or ScriptTask::handle_resize) then the iframe is cleared and never re-drawn.

@glennw
Copy link
Member

glennw commented Oct 15, 2015

I think you should be able to avoid calling update_subpage_size_if_necessary() by passing a result back from update_layer_if_exists() that says whether the size of rect was different (see the update_layer method in CompositorLayer).

@paulrouget
Copy link
Contributor Author

I think you should be able to avoid calling update_subpage_size_if_necessary() by passing a result back from update_layer_if_exists() that says whether the size of rect was different (see the update_layer method in CompositorLayer).

It works. But iframe is cleared as well.

@glennw
Copy link
Member

glennw commented Oct 15, 2015

OK, thanks for checking. I will debug with your repro in the morning and let you know what I find. I did briefly try your test case above on webrender and there is no flickering - although it doesn't clip the iframe correctly yet.

@glennw
Copy link
Member

glennw commented Oct 19, 2015

I can see the problem. It's not clear to me what the best fix is yet - @pcwalton may have some thoughts, since he's been working on this stuff lately. If not, I'll dig into it again tomorrow.

The iframe has a normal root layer and also a layer for the scrolling region. However, the scrolling layer doesn't have any subpage info associated with it so when the root pipeline supplies a new set of layers, the collect_old_layers functionality removes the scroll layer from the compositor.

Possible solutions might be (a) storing the subpage info in sub-layers so they aren't collected (b) passing some kind of "current owner" during recursion in collect_old_layers to avoid collecting sub-layers (c) perhaps collecting old layers should stop recursing once it gets to a subpage of the pipeline being considered?

I'm not sure if those options would have side effects or which would be preferable?

@paulrouget
Copy link
Contributor Author

I filed an issue for the rAF bug: #8075

@paulrouget
Copy link
Contributor Author

@glennw do you think the problem you describe also explains why this happen: #7864

@glennw
Copy link
Member

glennw commented Oct 20, 2015

@paulrouget Now that #8089 has landed, could you confirm that it fixes this issue, and check if it fixes the scroll issue you referenced above? (I believe it will also fix the iframe navigation regression issue you reported).

@paulrouget
Copy link
Contributor Author

#8089 fixes this issue, the scrolling issue as well. But the navigation one.

Thanks a lot @glennw !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants