1178591 - [e10s] Window resize event is triggered in all document in the same window

Status: RESOLVED WORKSFORME

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

Description

[Xidorn Quan [:xidorn] UTC+10]

On e10s window, when the window is resized, all tabs in the window receive the resize event even if it is not the current tab.

Step to reproduce:
1. open a page which listen to resize event
2. switch to another tab
3. resize the window

Expected result:
The background page shouldn't receive the resize event until we switch to it.

Actual result:
The background page receives the resize event.

This seems to be a regression in the current Nightly. I cannot reproduce the same behavior on the current Developer Edition.

Comment 1

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]:

We really don't want to ship this...

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

The STR in comment 0 doesn't seem to be correct. When I try again, it doens't happen when I resize the window, but happens when it enters/leaves DOM fullscreen or fullscreen mode. So the step to reproduce should be:

1. open a page which listen to resize event
2. switch to another tab
3. enter fullscreen mode (press F11)

With these steps, I can reproduce on both Developer Edition and Nightly, hence it is probably not a regression.

Comment 3

[Alice0775 White]

Attachment: testcase

STR with comment#0
I can reproduce the problem on Nightly41.0a1 an Aurora40.0a2 with e10s.

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5864129e5d5f&tochange=0160303649ae

Regressed by: 0160303649ae	David Parks — Bug 1075670 - Make event.screen[XY] work in content processes (r=smaug,kats,tn,joshmoz)

Comment 4

[Alice0775 White]

Unhide menubar(Press alt key) also triggers the resize event.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

FYI the changes I'm working on for bug 1178847 may fix this.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Backtrace to ResizeReflow

Unfortunately it looks like the patches on bug 1178847 don't fix this. I'm still seeing resize events hitting a background tab when I fullscreen/unfullscreen the foreground tab. Checked on Linux and OS X. I'm attaching a backtrace of the stack to PresShell:ResizeReflow (the first time it gets hit on the background tab); it appears to be going through some IME code that I'm not familiar with. There might be a second bug being exposed here, I'm not really sure.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Masayuki, do you know if this is expected behaviour? See backtrace in comment 6.

Comment 8

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

Isn't this about any layout flush triggering pending resize events.

Comment 9

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

And if so, the question would be why we have pending resize event for a background tab.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

PuppetWidget tries to send window move for adjusting composition window of IME. At this time, ContentCacheInChild tries to query content information with ContentEventHandler. Then, ContentEventHandler flushes the pending notifications first. So, it should be performed only on the focused window. However, NS_QUERY_* should be handled with active PresShell in the widget. So, I don't understand why this occurs...

Comment 11

[Kartikaya Gupta (email:[e-mail])]

(In reply to Olli Pettay [:smaug] (away-ish until July 26, expect slower than usual reviews) from comment #9)
> And if so, the question would be why we have pending resize event for a
> background tab.

Oh? I thought that the desired behavior was that the background tab gets a resize notification but it only gets processed (and the reflow only happens) after the tab is brought back into the foreground. That's what I observed happening when I tested bug 1158640 and it seemed like the ideal behavior.

Comment 12

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

Since resize event fires in current Gecko whenever layout is flushed, and scripts can easily flush
layout also in background tabs, I think postponing resize to happen later when tab becomes active would be better, I think.
And IIRC that is what happens in non-e10s - but worth to test.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

I did some investigation to wrap my head around what goes on. When the top-level widget is resized, all of the tab's nsFrameLoaders are notified. When it gets to [1], the non-e10s and e10s codepaths separate.

With non-e10s we call UpdateBaseWindowPositionAndSize, which trickles down into [2]. If the tab is in the foreground it gets updated right away, otherwise mDelayedResize is populated. Later, when the tab is brought into the foreground the FlushDelayedResize function is called which does a reflow *if* the new size is different from the old size. In the case of entering and exiting fullscreen, the new size is the same as the old size, and so it gets discarded. However, a background tab *can* still trigger a layout flush and resize event while a different tab is in the foreground. For example, using the test page on this bug, I ran the following in the console:
  setTimeout(function() { document.body.appendChild(document.createTextNode("hello")) }, 5000);
and then switched to a different tab and fullscreened. After 10 seconds I unfullscreened and came back to the test page tab, and observed that it had triggered two resize events (one for the flush that happened as a result of the document.appendChild while fullscreened, and one for resizing back to unfullscreened size when I brought it into the foreground).

This all seems basically in line with how I expected things to work - background tabs end up with delayed resizes, and a flush can cause a reflow and resize event to be dispatched at any time.

Going back to the e10s scenario at [1], we send an UpdateDimensions message to the child. As with the non-e10s scenario, this basically queues up a delayed resize if the tab is in the background, or reflows immediately if the tab is in the foreground. It uses the same machinery as the non-e10s scenario. The only difference is that after it does this, it immediately causes a layout flush via the ContentCacheChild codepath that I attached in comment 6. This is the equivalent of the setTimeout code that I ran manually, except that it happens unconditionally in the browser itself. I think the ContentCacheChild code should probably defer doing whatever it is doing if the tab is in the background.

I don't think I have the necessary knowledge of the ContentCacheChild code to fix this, but hopefully the above is enough to point one of you guys to the right fix.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFrameLoader.cpp?rev=0ac19d3bf7bf#1995
[2] http://mxr.mozilla.org/mozilla-central/source/view/nsViewManager.cpp?rev=a026b731506a#200

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Modern IME framework queries focused content. Therefore, TabParent need to cache whole content of the editor (ideally). For that, ContentCacheInChild retrieves content information when something is changed. If it works when focus isn't in any editor, it's a bug. However, if an editor has focus, ContentCacheInChild needs to query content (then, causes flushing the layout).

Comment 15

[Markus Stange [:mstange]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #14)
> If it works when focus isn't in any editor, it's a bug.

Editors in background tabs shouldn't have focus, so it sounds like this is a bug, then.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Markus Stange [:mstange] from comment #15)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #14)
> > If it works when focus isn't in any editor, it's a bug.
> 
> Editors in background tabs shouldn't have focus, so it sounds like this is a
> bug, then.

Yes. But perhaps, this bug must be reproduced when an editor in active tab has focus even if we make ContentCacheInChild not work when there is no focused editor.

Comment 17

[Markus Stange [:mstange]]

Hmm. Still, if you're flushing the document for an editor in the foreground tab, that flush shouldn't cause any delayed resizes for background tabs to be flushed, should it?

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Markus Stange [:mstange] from comment #17)
> Hmm. Still, if you're flushing the document for an editor in the foreground
> tab, that flush shouldn't cause any delayed resizes for background tabs to
> be flushed, should it?

I think it shouldn't cause. According to comment 12, we shouldn't push a resize event into the queue of background tabs.

Comment 19

[Jim Mathies [:jimm]]

Attachment: updated test case

Comment 20

[Jim Mathies [:jimm]]

I'm not able to reproduce this on Windows 7 with latest nightly.