1155494 - [e10s] Exiting fullscreen by press F11 takes 5 seconds

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Alice0775 White]

This happens only with e10s

Steps to reproduce:
1. Open http://www.msn.com/ja-jp
2. Press F11 Fullscreen
3. Press F11 Exit Fullscreen

Actual Results:
Exiting fullscreen by press F11 takes 5 seconds

Expected Results:
It should be instant


Regression window:
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44d89ddc145a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0 ID:20150215212944
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15dea2b0929b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0 ID:20150216005844
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=44d89ddc145a&tochange=15dea2b0929b

Suspect: Bug 1130920

Comment 1

[Alice0775 White]

Sorry Bug 1130051

Comment 2

[Alice0775 White]

Via local build
Last Good: 143174851bb7
First Bad: 416a3c508df2

Triggered by: 	416a3c508df2	David Parks — Bug 1130051 - Restore old semantics of IPDL 'compress' attribute. r=billm

Comment 3

[David Parks [:handyman]]

Bill,

A RefreshDriver observer (nsSynthMouseMoveEvent) is creating a fake mousemove on WillRefresh (SendRealMouseMove) that ends up in between each SendUpdateDimensions call so UpdateDimensions is never compressed anymore.  This resembles the issue that sparked the original IPDL compress change in bug 1076820 that we undid in bug 1130051.

I like the original solution, that uses a more aggressive definition of 'compress' (i.e. compress messages, even if they arent adjacent in the actor's message queue).  In bug 1130051, we dumped that so we could use the 'compress' attribute on mousemove messages, which kats said would otherwise have some bad edge cases.  Even if we need the old 'compress' for mousemoves, I'd recommend adding the other type of compress here (say, 'compressall').  Alternatives include trying to move/remove the intervening message (precarious and will just land us back here in a few months) or batch the UpdateDimensions calls (hard, as each one is in response to a separate resize event from Windows).  Thoughts?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Why do we have so many UpdateDimensions calls? Don't we just change the window size once?

I guess we could consider changing compress again. I'd like to understand the problem more though.

Comment 5

[David Parks [:handyman]]

Best I can tell, the idea behind the spurious mousemove events is that they allow the cursor to update after a reflow since the content under the cursor may have changed.  This is certainly logical.  I cant explain why it wasn’t happening before — the fake-mousemove-on-reflow code predates the mercurial switch — but it should have been (although cursors haven’t worked anyway).

PresShell::DidDoReflow initiates the mousemove here:
https://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#9015

Assuming we don’t change the compression semantics for mousemoves (only for updateDimensions), we would still get a useless block of synthesized mousemove events, but only the final one will make a difference.  It should work fine.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

OK, I guess we can do the compressall thing.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

I offered to take this from David.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Add a 'compressall' message flag.

David, this is what you were thinking of, right?

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Implement the compressall IPC message option.

Comment 12

[David Parks [:handyman]]

Comment on attachment 8600504 [details] [diff] [review]
Add a 'compressall' message flag.

Yes, this is what I had in mind.

A few quick bits:

>   // True if compression is enabled for this message.
>-  bool compress() const {
>+  MessageCompression compress_type() const {
>+    return (header()->flags & COMPRESS_BIT) ?
>+               COMPRESSION_ENABLED :
>+               (header()->flags & COMPRESSALL_BIT) ?
>+                   COMPRESSION_NONE :                      // (1)
>+                   COMPRESSION_NONE;
>+  }

I think you mean (1) to be COMPRESSION_ENABLED


>+
>+  // True if compressall is  enabled for this message.
>+  bool compressall() const {
>     return (header()->flags & COMPRESS_BIT) != 0;           // (2)
>   }

s/COMPRESS_BIT/COMPRESSALL_BIT

>+    } else if (aMsg.compress_type() == IPC::Message::COMPRESSION_ALL) {
>+        // TODO
>     }

I'm pretty sure you can cut-and-paste the code from the patch in bug 1076820 into the TODO block, unchanged.

(The rest of this the IPDL stuff is greek to me.)

Comment 13

[David Parks [:handyman]]

Didnt mean to click that NI.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Add a 'compressall' message flag

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Implement the compressall IPC message option.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8600984 [details] [diff] [review]
Implement the compressall IPC message option.

billm, mind re-stamping this? You already looked at it in bug 1076820.

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8600983 [details] [diff] [review]
Add a 'compressall' message flag

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

Thanks Blake.

::: ipc/glue/MessageChannel.cpp
@@ +652,5 @@
>          return;
>      }
>  
>      // Prioritized messages cannot be compressed.
> +    MOZ_ASSERT(aMsg.compress_type() == IPC::Message::COMPRESSION_NONE ||

Please use MOZ_ASSERT_IF(!COMPRESSION_NONE, PRIO_NORMAL).

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2085810a65e1 (I forgot to address comment 17 before pushing to try. I do have it fixed locally).

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/640dd49d3236

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/674495f6c760

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/640dd49d3236
https://hg.mozilla.org/mozilla-central/rev/674495f6c760