820571 - [fullscreen] resize event with window size 0 when leaving fullscreen

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[David Flanagan [:djf]]

In the Gaia gallery app, I use fullscreen mode to display photos, but transition out of fullscreen mode to display the list of photo thumbnails.

I find that this fullscreen transition is pretty flaky on MacOS and on my Unagi device. Sometimes I just get one resize event during the transition. But sometimes the window size goes to 0 and there is an extra resize event.

This setting the window size to 0 causes problems for the gallery because it forgets its scroll position in the list of thumbnail images, and I've had to implement a complicated workaround in the gallery app.

On my B2G device switching into fullscreen is also a flashy process (I see blank white screen for a bit), and I wonder if that could be related.

Comment 1

[Cervantes Yu [:cyu] [:cervantes]]

This looks to be OOP-related. Some observations:
- Disabling OOP for gallery makes the problem disappear (including resize event and white flash).
- The resize event is from PuppetWidget calling Resize() with width and height = 0.

Comment 2

[David Flanagan [:djf]]

Nominating this a blocking because the flash of white when going to fullscreen is kind of bad, and it sounds like we have a lead on getting it fixed here...

Comment 3

[Andrew Overholt [:overholt]]

Let's try to fix this but we can't hold the release for it.

Comment 4

[Cervantes Yu [:cyu] [:cervantes]]

The white blank screen and the extra resize event is the result of PuppetWidget resizing to 0. The result is we have an empty window. That is why a white blank screen is seen and an extra resize event is fired.

When we enter or leave fullscreen, the iframe is restyled. nsCSSFrameConstructor recreate frames for content. The a new nsSubDocumentFrame is created and initialized for content. Then nsSubDocumentFrame::ShowViewer() is called, but since it hasn't been reflowed, its mRect is still empty. Then nsFrameLoader::ShowRemoteFrame() is called with empty size, making the PuppetWidget in the child process to be shinked to empty.

Proposals to fix:
1. Don't call nsSubDocumentFrame::ShowViewer() until reflow finished.
2. Don't reconstruct frames for <iframe mozbrowser remote="true"/> elements.

Comment 5

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Show the viewer after reflow finished for remote iframes

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 698617 [details] [diff] [review]
Show the viewer after reflow finished for remote iframes

Review of attachment 698617 [details] [diff] [review]:
-----------------------------------------------------------------

Good diagnosis and patch, but this is quite dangerous since it extends the range of time during which the presentation is stashed away.

It would be a lot safer to call ShowViewer in AsyncFrameInit as now, but lie to the subdocument about its size until the next reflow has completed. Can you try that?

Comment 7

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Don't show remote iframe with empty size

Use a safer alternative: use size from an ancestor temporarily in nsSubDocumentFrame::Init(). Tests show pretty good results. The size set in ::Init() is actually the size after reflow in the gallery app.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 699093 [details] [diff] [review]
Don't show remote iframe with empty size

Review of attachment 699093 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsSubDocumentFrame.cpp
@@ +180,5 @@
> +        break;
> +      }
> +      frame = frame->GetParent();
> +    }
> +  }

This is somewhat nasty. It violates some layout invariants (such as that a frame's visual and scrollable overflow areas contain its border-box).

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: outline of fix

This is more like what I had in mind.

I haven't built or tested this patch, and some IIDs need to be revved --- please finish it :-).

Comment 10

[Cervantes Yu [:cyu] [:cervantes]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> 
> This is more like what I had in mind.
> 
This patch generates (10, 10) resize events because we can't get a detached subdocview from the frame loader. The result is we still get a (almost, the content is shrinked to (10, 10)) white blank screen when entering full screen. We might need to find another source of size if we want to get rid of the strange blank screen.

One question: what kind of danger will it be if we extend the period the presentation is stashed?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Cervantes Yu from comment #10)
> This patch generates (10, 10) resize events because we can't get a detached
> subdocview from the frame loader. The result is we still get a (almost, the
> content is shrinked to (10, 10)) white blank screen when entering full
> screen. We might need to find another source of size if we want to get rid
> of the strange blank screen.

I see.

> One question: what kind of danger will it be if we extend the period the
> presentation is stashed?

Crashes.

How about, when we can't find a size, we use a size of -1,-1 and make the methods that pass the size down to the content process treat -1,-1 as "don't know" and don't resize the content process view in that case?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe nsFrameLoader::ShowRemoteFrame can just skip calling mRemoteFrame->UpdateDimensions if the size is unknown. We'll eventually get an nsFrameLoader::UpdatePositionAndSize with a real size when the reflow happens.

Comment 13

[Cervantes Yu [:cyu] [:cervantes]]

This sounds a good fix. I'll give it a try.

Comment 14

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Don't show remote iframe with empty size (v2)

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 700317 [details] [diff] [review]
Don't show remote iframe with empty size (v2)

Review of attachment 700317 [details] [diff] [review]:
-----------------------------------------------------------------

I wrote most of this patch, so we need another reviewer as well.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 700317 [details] [diff] [review]
Don't show remote iframe with empty size (v2)

Review of attachment 700317 [details] [diff] [review]:
-----------------------------------------------------------------

Fishing for a quick review here.

Comment 17

[Cervantes Yu [:cyu] [:cervantes]]

Test results on try server:
https://tbpl.mozilla.org/?tree=Try&rev=455de7ea85d0

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c232dd64f733

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Cervantes, can you check if this patch fixes bug 823327 and/or bug 825848?

If it does, you should let them know you fixed the bug :-)

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c232dd64f733

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 700317 [details] [diff] [review]
Don't show remote iframe with empty size (v2)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: Nasty-looking transitions in and out of fullscreen
Testing completed: just landed on central
Risk to taking this patch (and alternatives if risky): It is not zero-risk, but it is fairly low-risk. We carefully designed the patch to be conservative, so it doesn't mess with the lifetime of objects or anything like that. It affects only the size used to lay out subdocuments and skips a call to mRemoteBrowser::UpdateDimensions for subframes that haven't been reflowed yet.
String or UUID changes made by this patch: None.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> Cervantes, can you check if this patch fixes bug 823327 and/or bug 825848?
> 
> If it does, you should let them know you fixed the bug :-)

Cervantes?

Comment 23

[Cervantes Yu [:cyu] [:cervantes]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> > Cervantes, can you check if this patch fixes bug 823327 and/or bug 825848?
> > 
> > If it does, you should let them know you fixed the bug :-)
> 
> Cervantes?

Sorry that I overlooked the comment. Bug 823327 is the same bug. 825848 isn't, where tabchild didn't receive resize 0 request in changing orientation.

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

This isn't blocking-basecamp:+ or blocking-b2g:tef+, so we'll get this into v1.0.1. Feel free to land to the shira Gecko branch, and mark the status-b2g18:fixed (given the fact that shira will merge to mozilla-b2g18).

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 700317 [details] [diff] [review]
Don't show remote iframe with empty size (v2)

Please go ahead with uplift to the tip of mozilla-b2g18 branch for v1.0.1

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/6784023e2ab4

(This required a little finessing due to the lack of bug 826632 on b2g18)