1005405 - OverflowChangedTracker sometimes doesn't update all of the frame

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[mdew]

Attachment: test.rar

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 (Beta/Release)
Build ID: 20140502134938

Steps to reproduce:

Using the test case (hover over the "dfgdfg", and expect the menu everytime)


Actual results:

Doesn't popup.


Expected results:

Menu should popup when you slide mouse over it. Previously reported and fixed: https://bugzilla.mozilla.org/show_bug.cgi?id=766843

Did work, and recently broke in nightly.

Comment 1

[mdew]

Regression;

Index of /pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1397654043
16-Apr-2014 14:01 GOOD

Index of /pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1397700730
17-Apr-2014 03:38  BAD

Comment 2

[mdew]

Last good revision: f3a457bc88a2
First bad revision: 746ce8a34f0f
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f3a457bc88a2&tochange=746ce8a34f0f

Comment 3

[:Gijs (he/him)]

Looks like bug 984226 to me. Kip, can you have a look?

Comment 4

[:kip (Kearwood Gilbert)]

Will review this today

Comment 5

[:kip (Kearwood Gilbert)]

I have reproduced the issue and confirmed that the behavior changed with the patch to Bug 984226

Comment 6

[:kip (Kearwood Gilbert)]

Attachment: V1 Fix For Bug 1005405,r=dbaron

This bug corrects an issue in OverflowChangedTracker, which resulted in an incomplete update of frames when multiple changes to different frames occurred simultaneously.

Comment 7

[:kip (Kearwood Gilbert)]

(In reply to :kip (Kearwood Gilbert) from comment #6)
> Created attachment 8418205 [details] [diff] [review]
> V1 Fix For Bug 1005405
> 
> This bug corrects an issue in OverflowChangedTracker, which resulted in an
> incomplete update of frames when multiple changes to different frames
> occurred simultaneously.

This has been pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=092cd5eedf37

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8418205 [details] [diff] [review]
V1 Fix For Bug 1005405,r=dbaron

r=dbaron, but it would be good to add a reftest as well

Comment 9

[:kip (Kearwood Gilbert)]

Attachment: V1 Reftest for Bug 1005405, r=dbaron

This reftest is a highly distilled version of the original testcase, reproducing the scenario where the parent frame is not updated completely by OverflowChangedTracker::Flush()

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Does this test fail without the patch?  (I'm not seeing where the *two* overflow changes from style are, nor what it is that depends on overflow area being correct.)

Comment 11

[:kip (Kearwood Gilbert)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #10)
> Does this test fail without the patch?  (I'm not seeing where the *two*
> overflow changes from style are, nor what it is that depends on overflow
> area being correct.)

Yes, without the patch, the text will not be visible.  Tables are one case that still depends on the OverflowChangedTracker's original behavior, which expects the parent to always be updated, even if the overflow didn't change for the child.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(I'm assuming you tested that the reftest harness reports a failure, not just visually.  With invalidation tests like this, that's worth checking, although for other sorts of tests checking visually is fine.)

Comment 13

[:kip (Kearwood Gilbert)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #12)
> (I'm assuming you tested that the reftest harness reports a failure, not
> just visually.  With invalidation tests like this, that's worth checking,
> although for other sorts of tests checking visually is fine.)

I performed a test through the reftest harness on OSX 10.9.2 / Intel; it failed (as expected) without the patch and passed with the patch present.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/446230ff2bdb
https://hg.mozilla.org/integration/mozilla-inbound/rev/50e88d51767c

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/446230ff2bdb
https://hg.mozilla.org/mozilla-central/rev/50e88d51767c

Comment 16

[mdew]

Did this bug make it into aurora/upcoming FF30 ? 

Also might be a good idea to modify the title to be a bit more descriptive of the issue.

Comment 17

[:Gijs (he/him)]

(In reply to mdew from comment #16)
> Did this bug make it into aurora/upcoming FF30 ? 

Aurora, I think. kip, do you want to request uplift or is this not safe?

> Also might be a good idea to modify the title to be a bit more descriptive
> of the issue.

Good point.

Comment 18

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #17)
> (In reply to mdew from comment #16)
> > Did this bug make it into aurora/upcoming FF30 ? 
> 
> Aurora, I think. kip, do you want to request uplift or is this not safe?

+needinfo

Comment 19

[:kip (Kearwood Gilbert)]

(In reply to :Gijs Kruitbosch from comment #18)
> (In reply to :Gijs Kruitbosch from comment #17)
> > (In reply to mdew from comment #16)
> > > Did this bug make it into aurora/upcoming FF30 ? 
> > 
> > Aurora, I think. kip, do you want to request uplift or is this not safe?
> 
> +needinfo

I believe this patch is quite safe and worthy of uplift

Comment 20

[:kip (Kearwood Gilbert)]

Comment on attachment 8418205 [details] [diff] [review]
V1 Fix For Bug 1005405,r=dbaron

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 984226
User impact if declined: Some frames may fail to update, resulting in incorrect overflow area calculation or invisible elements.  These issues are not specific to sticky elements.
Testing completed (on m-c, etc.): Manual testing by performing DOM changes through Javascript as well as automated testing on m-c.  A reftest has been added to verify the fix and the patch has been pushed to the try server. 
Risk to taking this patch (and alternatives if risky): Medium risk.  This patch will result in more work being done while reflowing (affects change hint propagation to parent frames).  It is a simple, single line change in a class (OverflowChangeTracker) that impacts all elements during reflow.  If the patch is flawed, it could result in performance regressions.
String or IDL/UUID changes made by this patch: None

Comment 21

[Paul Silaghi, QA [:pauly]]

Verified fixed 32.0a1 (2014-05-08), Win 7 x64

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/54f1c073c4b5
https://hg.mozilla.org/releases/mozilla-aurora/rev/dceb849a2d71

Comment 23

[Bogdan Maris, Desktop QA]

Reproduced the initial issue on old Nightly (2014-05-02), verified as fixed on Windows 7 64bit using Firefox 31 beta 6.