1267568 - [e10s] UpdateDimensions message may trigger additional ResizeReflow for fullscreen change

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+10]

We only want one flush for fullscreen change in general. We ensure that in non-e10s (on systems other than Linux) via setting flag in pres shell to stop any immature resize reflow before we finish changing the document state, so that every thing would be updated together in the next tick.

This doesn't work perfectly with e10s. If UpdateDimensions message arrives before the next tick, a ResizeReflow would be triggered immediately, in which moment, the fullscreenchange event has not been dispatched, so the document could still be changing, and thus there could be another consequent flush. This would eventually make the change take too much time.

We need to find a way to either defer sending the UpdateDimensions message in the parent process, or suppress the effect of that message in the content process somehow.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

It seems for exiting fullscreen, UpdateDimensions could even be sent earlier than the CleanUp message.

Comment 2

[Jim Mathies [:jimm]]

Xidorn, is this something you can pick up? Also, would you consider this a blocker for e10s rollout?

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Yes, I can pick up, but I don't have clear idea how to fix it yet. If this affects real life cases, then probably yes. It seems to me Youtube could be affected.

Comment 4

[Jim Mathies [:jimm]]

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #3)
> Yes, I can pick up, but I don't have clear idea how to fix it yet. If this
> affects real life cases, then probably yes. It seems to me Youtube could be
> affected.

What are the side effects, slow full screen transitions? I've been testing this this morning and aside from bug 1266799 transitioning feels pretty smooth.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Yes, slow fullscreen transition.

I observed this while profiling, but without that, it seems most of the time everything works just fine. It is actually a race condition: if there is something happens to be slow before, this issue would make things even worse, so I guess it's worth a fix. Also it would allow me to continue profiling and see if there is anything else actionable.

I have had an idea how to fix it, and the fix shouldn't be too complicate. If everything goes fine, I should be able to submit the patchset for review tomorrow.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1267568 part 1 - Add a weak ptr to nsGlobalChromeWindow to remember the pres shell we set the fullscreen change flag. r=smaug

This addresses the review comment from bug 1177155 comment 16 so that
the assertion and code to avoid breaking assertion in valid path are no
longer needed.

Review commit: https://reviewboard.mozilla.org/r/49395/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49395/

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1267568 part 2 - Add helper method to nsIPresShell to get the RefreshDriver. r=smaug

Review commit: https://reviewboard.mozilla.org/r/49397/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49397/

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1267568 part 3 - Use an individual method for check whether resize should be delayed. r=smaug

Review commit: https://reviewboard.mozilla.org/r/49399/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49399/

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1267568 part 4 - Replace InFullscreenChange flag of PresShell with IsResizeSuppressed flag in RefreshDriver. r=smaug

Review commit: https://reviewboard.mozilla.org/r/49401/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49401/

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1267568 part 5 - Replace the old FullscreenChangePrepare class with a single function. r=smaug

Review commit: https://reviewboard.mozilla.org/r/49403/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49403/

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1267568 part 6 - Send fullscreen message before triggering any potential reflow. r=dao

Review commit: https://reviewboard.mozilla.org/r/49405/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49405/

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8746396 [details]
MozReview Request: Bug 1267568 part 6 - Send fullscreen message before triggering any potential reflow. r=dao

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49405/diff/1-2/

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8746391 [details]
MozReview Request: Bug 1267568 part 1 - Add a weak ptr to nsGlobalChromeWindow to remember the pres shell we set the fullscreen change flag. r=smaug

https://reviewboard.mozilla.org/r/49395/#review46229

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8746392 [details]
MozReview Request: Bug 1267568 part 2 - Add helper method to nsIPresShell to get the RefreshDriver. r=smaug

https://reviewboard.mozilla.org/r/49397/#review46231

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8746393 [details]
MozReview Request: Bug 1267568 part 3 - Use an individual method for check whether resize should be delayed. r=smaug

https://reviewboard.mozilla.org/r/49399/#review46233

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8746394 [details]
MozReview Request: Bug 1267568 part 4 - Replace InFullscreenChange flag of PresShell with IsResizeSuppressed flag in RefreshDriver. r=smaug

https://reviewboard.mozilla.org/r/49401/#review46241

make sure to test both e10s and non-e10s

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8746395 [details]
MozReview Request: Bug 1267568 part 5 - Replace the old FullscreenChangePrepare class with a single function. r=smaug

https://reviewboard.mozilla.org/r/49403/#review46247

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8746391 [details]
MozReview Request: Bug 1267568 part 1 - Add a weak ptr to nsGlobalChromeWindow to remember the pres shell we set the fullscreen change flag. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49395/diff/1-2/

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8746392 [details]
MozReview Request: Bug 1267568 part 2 - Add helper method to nsIPresShell to get the RefreshDriver. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49397/diff/1-2/

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8746393 [details]
MozReview Request: Bug 1267568 part 3 - Use an individual method for check whether resize should be delayed. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49399/diff/1-2/

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8746394 [details]
MozReview Request: Bug 1267568 part 4 - Replace InFullscreenChange flag of PresShell with IsResizeSuppressed flag in RefreshDriver. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49401/diff/1-2/

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8746395 [details]
MozReview Request: Bug 1267568 part 5 - Replace the old FullscreenChangePrepare class with a single function. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49403/diff/1-2/

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8746396 [details]
MozReview Request: Bug 1267568 part 6 - Send fullscreen message before triggering any potential reflow. r=dao

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49405/diff/2-3/

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

https://reviewboard.mozilla.org/r/49395/#review46425

::: dom/base/nsGlobalWindow.cpp:6285
(Diff revisions 1 - 2)
>    if (!NS_WARN_IF(!IsChromeWindow())) {
>      auto chromeWin = static_cast<nsGlobalChromeWindow*>(this);
> -    if (MOZ_LIKELY(!chromeWin->mFullscreenPresShell)) {
> +    if (!NS_WARN_IF(chromeWin->mFullscreenPresShell)) {
>        if (nsIPresShell* shell = mDocShell->GetPresShell()) {
>          chromeWin->mFullscreenPresShell = do_GetWeakReference(shell);
>          MOZ_ASSERT(chromeWin->mFullscreenPresShell);
>          shell->SetIsInFullscreenChange(true);
>        }
> -    } else {
> -      MOZ_ASSERT_UNREACHABLE("We should not have been in fullscreen change");
>      }
>    }

This is the only relevant part in the interdiff.

Since there was a crash from this assertion in try push, I changed this assertion to a warning. Olli, are you fine with this change?

I guess it could happen since the window size sometimes changes asynchronously, and thus there is a chance this function could be called before the previous fullscreen change finishes.

The test triggers this assertion is browser/components/extensions/test/browser/browser_ext_windows_update.js which changes the window state quickly synchronously, which doesn't seem to be something could happen in general.

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

Looks good on try push now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f46ba7b969e9

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8746391 [details]
MozReview Request: Bug 1267568 part 1 - Add a weak ptr to nsGlobalChromeWindow to remember the pres shell we set the fullscreen change flag. r=smaug

https://reviewboard.mozilla.org/r/49395/#review46935

MozReview failed totally with interdiff here :/

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Olli Pettay [:smaug] from comment #26)
> Comment on attachment 8746391 [details]
> MozReview Request: Bug 1267568 part 1 - Add a weak ptr to
> nsGlobalChromeWindow to remember the pres shell we set the fullscreen change
> flag. r=smaug
> 
> https://reviewboard.mozilla.org/r/49395/#review46935
> 
> MozReview failed totally with interdiff here :/

Yeah, because they turned off rebase filter completely. But I've quoted the only changed part in comment 24, which hopefully made your review easier.

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd0fd7a876d62da427a2d4587e13f1837e1b78c7
Bug 1267568 part 1 - Add a weak ptr to nsGlobalChromeWindow to remember the pres shell we set the fullscreen change flag. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa640b84845393a371b96a5bc5d17147ecff9df5
Bug 1267568 part 2 - Add helper method to nsIPresShell to get the RefreshDriver. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffcdd861062d2400e3991964bb151ad17f270faa
Bug 1267568 part 3 - Use an individual method for check whether resize should be delayed. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/da3006dcfb7b7525ef19208d2d38d52c01f72bb3
Bug 1267568 part 4 - Replace InFullscreenChange flag of PresShell with IsResizeSuppressed flag in RefreshDriver. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/522031a5febb97d77c84e967c9ee57262c1ef7bb
Bug 1267568 part 5 - Replace the old FullscreenChangePrepare class with a single function. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/d95633a61869a008b69da8d621c0d0d127874fe5
Bug 1267568 part 6 - Send fullscreen message before triggering any potential reflow. r=dao

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dd0fd7a876d6
https://hg.mozilla.org/mozilla-central/rev/fa640b848453
https://hg.mozilla.org/mozilla-central/rev/ffcdd861062d
https://hg.mozilla.org/mozilla-central/rev/da3006dcfb7b
https://hg.mozilla.org/mozilla-central/rev/522031a5febb
https://hg.mozilla.org/mozilla-central/rev/d95633a61869

Comment 30

[Xidorn Quan [:xidorn] UTC+10]

I wonder whether I should request uplifting it to aurora. The improvement could be just marginally in most cases. It's probably not worth considering this isn't a trivial change.

Comment 31

[Jim Mathies [:jimm]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #30)
> I wonder whether I should request uplifting it to aurora. The improvement
> could be just marginally in most cases. It's probably not worth considering
> this isn't a trivial change.

It didn't alter talos test results afaict so the e10s team wont push for it. Up to you.