Closed Bug 1155494 Opened 9 years ago Closed 9 years ago

[e10s] Exiting fullscreen by press F11 takes 5 seconds


(Core :: IPC, defect)

40 Branch
Windows 7
Not set



Tracking Status
e10s m6+ ---
firefox40 --- fixed


(Reporter: alice0775, Assigned: mrbkap)


(Blocks 1 open bug)


(Keywords: perf, regression)


(2 files, 2 obsolete files)

This happens only with e10s

Steps to reproduce:
1. Open
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:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0 ID:20150215212944
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0 ID:20150216005844

Suspect: Bug 1130920
Flags: needinfo?(davidp99)
Sorry Bug 1130051
Blocks: 1130051
No longer blocks: 1130920
Keywords: perf
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
Component: DOM → IPC

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?
Flags: needinfo?(davidp99) → needinfo?(wmccloskey)
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.
Flags: needinfo?(wmccloskey) → needinfo?(davidp99)
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:

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.
Flags: needinfo?(davidp99) → needinfo?(wmccloskey)
OK, I guess we can do the compressall thing.
Flags: needinfo?(wmccloskey)
Assignee: nobody → davidp99
I offered to take this from David.
Assignee: davidp99 → mrbkap
David, this is what you were thinking of, right?
Attachment #8600504 - Flags: feedback?(davidp99)
Attachment #8600505 - Flags: feedback?(davidp99)
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)
>   }


>+    } 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.)
Flags: needinfo?(mrbkap)
Attachment #8600504 - Flags: feedback?(davidp99)
Didnt mean to click that NI.
Flags: needinfo?(mrbkap)
Attachment #8600505 - Flags: feedback?(davidp99)
Attachment #8600504 - Attachment is obsolete: true
Attachment #8600505 - Attachment is obsolete: true
Attachment #8600983 - Flags: review?(wmccloskey)
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.
Attachment #8600984 - Flags: review?(wmccloskey)
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 ||

Attachment #8600983 - Flags: review?(wmccloskey) → review+
Attachment #8600984 - Flags: review?(wmccloskey) → review+ (I forgot to address comment 17 before pushing to try. I do have it fixed locally).
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
QA Whiteboard: [good first verify][verify in Nightly only]
Blocks: 1251707
You need to log in before you can comment on or make changes to this bug.