Closed Bug 753329 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox for Android :: General, defect)

14 Branch
ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 16
Tracking Status
firefox14 + verified
firefox15 --- verified
blocking-fennec1.0 --- +
fennec 15+ ---

People

(Reporter: aakashd, Assigned: roc)

References

(Blocks 1 open bug, )

Details

(Keywords: regression, Whiteboard: [Engagement] [layout])

Attachments

(3 files, 1 obsolete file)

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.
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?
(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.
Is this a regression?
(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).
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
blocking-fennec1.0: --- → ?
Assignee: nobody → joe
blocking-fennec1.0: ? → +
(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.
The first bad revision is:
changeset:   91825:6dbdb799fa6d
user:        Robert O'Callahan <robert@ocallahan.org>
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
Assignee: joe → roc
Blocks: 728983
BTW, it's way easier to test this site if you turn off cookies in your Fennec.
This is still a blocker.
Whiteboard: [Engagement] → [Engagement] [layout]
Duplicate of this bug: 757324
There is a reduced testcase for this on bug 757324.
Attached file slightly more reduced testcase (obsolete) —
I've whittled down Martijn's testcase a little bit more.
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?
Alice, could you possibly find out when exactly the testcase of comment #12 got fixed on desktop?
(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
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.
(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.
That version of the testcase is still not working for me on Fennec nightly. How about you, JP?
tracking-fennec: --- → 15+
blocking-fennec1.0: + → -
blocking-fennec1.0: - → +
(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.
Duplicate of this bug: 761243
The first bad revision is:
changeset:   91825:6dbdb799fa6d
user:        Robert O'Callahan <robert@ocallahan.org>
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
Which I already knew, if I'd looked at comment 7....
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.
Attached patch fixSplinter Review
Attachment #630836 - Flags: review?(matt.woodrow)
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.
Attachment #630836 - Flags: review?(matt.woodrow) → review+
(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
Does this patch affect desktop as well?
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.
Hmm, this causes the testcase for bug 728983 to fail. I should have tested that manually first. At least the test is working though!
That followup passes the test here and in bug 728983.
Attachment #630861 - Flags: review?(matt.woodrow) → review+
Roc, can you nom this for beta/aurora with a risk assessment?  Also, does this impact desktop?
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.
Attachment #630836 - Flags: approval-mozilla-beta?
Attachment #630836 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/4f28fd1e2d81

(Merged by Ed Morley)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Comment on attachment 630836 [details] [diff] [review]
fix

please push this along with the test fix
Attachment #630836 - Flags: approval-mozilla-beta?
Attachment #630836 - Flags: approval-mozilla-beta+
Attachment #630836 - Flags: approval-mozilla-aurora?
Attachment #630836 - Flags: approval-mozilla-aurora+
(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?
Keywords: qawanted
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
(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.
Keywords: qawanted
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.
Status: RESOLVED → VERIFIED
Depends on: 775163
Depends on: 775592
Depends on: 870764
No longer depends on: 870764
You need to log in before you can comment on or make changes to this bug.