753329 - Content window doesn't re-paint with closed banner ads on trulia.com

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aakash Desai [:aakashd]]

Build Id: 5/9/2012 Aurora

Device: LG Revolution

Steps to Reproduce:
1. Go to trulia.com
2. Click "No thanks" on the Download App banner ad.

Actual Results:
The navbar part of the page is re-painted immediately, but most of the page still shows the ad. I have to wait about 2-3 seconds before the entire page is refreshed. 

Expected Results:
The ad immediately disappears and shows trulia's mobile site. 


Note: I've seen this on fandango and rottentomatoes.com as well. Stock browser works much better in comparison.

Comment 1

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

Clicking the "No thanks" just hides the interstitial ad (which is shown at z-index: 4, higher than the actual page content) by setting display:none on the div. This should be handled entirely within gecko's normal invalidation/repaint code. (And it does work on desktop).

CC'ing ali for any thoughts; I'm guessing multiple layers are created for the different z-indices on the content and maybe something isn't getting invalidated/composited properly?

Comment 2

[Ali Juma [:ajuma]]

(In reply to Kartikaya Gupta (:kats) from comment #1)
> CC'ing ali for any thoughts; I'm guessing multiple layers are created for
> the different z-indices on the content and maybe something isn't getting
> invalidated/composited properly?

It looks like we're not invalidating properly, since zooming fixes it immediately.

Comment 3

[Timothy Nikkel (:tnikkel)]

Is this a regression?

Comment 4

[Ali Juma [:ajuma]]

(In reply to Timothy Nikkel (:tn) from comment #3)
> Is this a regression?

Yes, it seems to be, since I cannot reproduce this using March 20th's Nightly (but I haven't tried to bisect it further).

Comment 5

[Adrian Tamas (:AdrianT)]

Issue is not reproducible on Nightly 2012-04-17 and is reproducible on Nightly 2012-04-18 0c7e2911be75

Possible regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c61e7c3a232a&tochange=0c7e2911be75

Comment 6

[Timothy Nikkel (:tnikkel)]

(In reply to adrian tamas from comment #5)
> Possible regression range:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=c61e7c3a232a&tochange=0c7e2911be75

In that range bug 728983 is the best first guess.

Comment 7

[Joe Drew (not getting mail)]

The first bad revision is:
changeset:   91825:6dbdb799fa6d
user:        Robert O'Callahan <[e-mail]>
date:        Tue Apr 17 17:45:04 2012 +1200
summary:     Bug 728983. Part 2: When display items for multiple frames are merged, track the merged frames and mark them all as having an associated container layer. This ensures that invalidations are processed correctly. r=mattwoodrow

Comment 8

[Joe Drew (not getting mail)]

BTW, it's way easier to test this site if you turn off cookies in your Fennec.

Comment 9

[Joe Drew (not getting mail)]

This is still a blocker.

Comment 11

[Joe Drew (not getting mail)]

There is a reduced testcase for this on bug 757324.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: slightly more reduced testcase

I've whittled down Martijn's testcase a little bit more.

Comment 13

[JP Rosevear [:jpr]]

This works in nightly for me on desktop and mobile.  Does not work on released desktop or aurora for mobile.  Just missing a patch to uplift?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Alice, could you possibly find out when exactly the testcase of comment #12 got fixed on desktop?

Comment 15

[Alice0775 White]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> Alice, could you possibly find out when exactly the testcase of comment #12
> got fixed on desktop?

Fixed range(M-c)
Reproduced the issue:
http://hg.mozilla.org/mozilla-central/rev/b9655f07e284
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120322 Firefox/14.0a1 ID:20120322142919
Fixed:
http://hg.mozilla.org/mozilla-central/rev/70ac5065caee
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120322 Firefox/14.0a1 ID:20120322180135
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b9655f07e284&tochange=70ac5065caee


Fixed range(M-i)
Reproduced the issue:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4790b56fe677
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120321 Firefox/14.0a1 ID:20120321201820
Fixed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/30fec3c6c966
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120321 Firefox/14.0a1 ID:20120321211119
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4790b56fe677&tochange=30fec3c6c966

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You are amazing.

However, I am stupid. I wrote the testcase to depend on the global scope polluter working in standards mode, which is not true on trunk. Bad testcase.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> which is not true on trunk.

I mean, it's not true in FF12.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: correct testcase

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That version of the testcase is still not working for me on Fennec nightly. How about you, JP?

Comment 20

[JP Rosevear [:jpr]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> That version of the testcase is still not working for me on Fennec nightly.
> How about you, JP?

Correct, updated test case fails on mobile nightly, works on desktop nightly.

Comment 22

[Joe Drew (not getting mail)]

Testing from nightlies proves this regressed:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c61e7c3a232aad0d8fb1bc2455487ce61dd9e182&tochange=0c7e2911be75b12aa98e83800231cecf127ec997

Currently doing a hg bisect.

Comment 23

[Joe Drew (not getting mail)]

The first bad revision is:
changeset:   91825:6dbdb799fa6d
user:        Robert O'Callahan <[e-mail]>
date:        Tue Apr 17 17:45:04 2012 +1200
summary:     Bug 728983. Part 2: When display items for multiple frames are merged, track the merged frames and mark them all as having an associated container layer. This ensures that invalidations are processed correctly. r=mattwoodrow

Comment 24

[Joe Drew (not getting mail)]

Which I already knew, if I'd looked at comment 7....

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Right, so this is a flat-out regression from that patch. In the comment #18 testcase with a displayport, we build an nsDisplayScrollLayer for both the page background and the abs-pos element background and merge them into a single layer. Its ThebesLayer child has the abs-pos element background drawn into it. Two frames get independent ThebesLayerInvalidRegionPropertys. When we hide the abs-pos element, its frame's ThebesLayerInvalidRegionProperty is updated with the frame's overflow area. Then when we update the layer tree. ApplyThebesLayerInvalidation is not called for the abs-pos element since it has nothing in the display list, so the ThebesLayerInvalidRegionProperty for the abs-pos frame is ignored. So we don't update the ThebesLayer.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 28

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 630836 [details] [diff] [review]
fix

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

Looks good, apart from the 753329-1-ref.html being missing.

I think this will work fine under DLBI.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #28)
> Looks good, apart from the 753329-1-ref.html being missing.

Er yeah. It's supposed to compare to about:blank.

Re-pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=25be4f6eb8ed

Comment 30

[JP Rosevear [:jpr]]

Does this patch affect desktop as well?

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it could in extremely rare cases involving dynamic changes to elements with 'opacity', 'clip-path', 'mask' or 'filter' and that are broken by line or column breaks. I wouldn't be surprised if it never shows up on real pages though.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, this causes the testcase for bug 728983 to fail. I should have tested that manually first. At least the test is working though!

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: followup to fix test failure

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That followup passes the test here and in bug 728983.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I folded the patches together and pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f28fd1e2d81

Comment 36

[JP Rosevear [:jpr]]

Roc, can you nom this for beta/aurora with a risk assessment?  Also, does this impact desktop?

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 630836 [details] [diff] [review]
fix

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

Fixes gnarly invalidation bug. The two patches together are pretty safe IMHO.

I expect that this bug does affect desktop but only in very rare situations. It's much more important on mobile.

Comment 38

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/4f28fd1e2d81

(Merged by Ed Morley)

Comment 39

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 630836 [details] [diff] [review]
fix

please push this along with the test fix

Comment 40

[Alex Keybl [:akeybl]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away June 9-19) from comment #37)
> I expect that this bug does affect desktop but only in very rare situations.
> It's much more important on mobile.

Are these scenarios something that we can point QA at on desktop?

Comment 41

[Joe Drew (not getting mail)]

https://hg.mozilla.org/releases/mozilla-beta/rev/7b9ce6660818
https://hg.mozilla.org/releases/mozilla-aurora/rev/a125034a52b3

Comment 42

[Joe Drew (not getting mail)]

I hastily backed this out of beta because I thought it caused build problems, then discovered the build problem was likely bug 753223.

https://hg.mozilla.org/releases/mozilla-beta/rev/7d4728a96cdd

So I re-pushed it:

https://hg.mozilla.org/releases/mozilla-beta/rev/1d132f9b0083

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Alex Keybl [:akeybl] from comment #40)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away June 9-19)
> from comment #37)
> > I expect that this bug does affect desktop but only in very rare situations.
> > It's much more important on mobile.
> 
> Are these scenarios something that we can point QA at on desktop?

Not really no. There's an automated test in the patch, and you can verify on mobile using the steps in comment #0.

Comment 44

[Adrian Tamas (:AdrianT)]

Verified fixed on:
Aurora 15.0a2 2012-07-08/Nightly 16.0a1 2012-07-08/Firefox Mobile 14.0b11
HTC Desire
Android 2.2.2

The issue described in the bug is not reproducible. Also the test case in the bug works correctly. The duplicated bugs - bug 757324 and bug 761243 - also are no longer reproducible.