Closed
Bug 753329
Opened 13 years ago
Closed 12 years ago
Content window doesn't re-paint with closed banner ads on trulia.com
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox14+ verified, firefox15 verified, blocking-fennec1.0 +, fennec15+)
VERIFIED
FIXED
Firefox 16
People
(Reporter: aakashd, Assigned: roc)
References
()
Details
(Keywords: regression, Whiteboard: [Engagement] [layout])
Attachments
(3 files, 1 obsolete file)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
mattwoodrow
:
review+
blassey
:
approval-mozilla-aurora+
blassey
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
Is this a regression?
Comment 4•13 years ago
|
||
(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).
Keywords: regressionwindow-wanted
Comment 5•13 years ago
|
||
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
Updated•13 years ago
|
Keywords: regressionwindow-wanted → regression
Updated•13 years ago
|
blocking-fennec1.0: --- → ?
Updated•13 years ago
|
Assignee: nobody → joe
blocking-fennec1.0: ? → +
Comment 6•13 years ago
|
||
(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.
Updated•13 years ago
|
URL: http://trulia.com
Comment 7•13 years ago
|
||
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
Comment 8•13 years ago
|
||
BTW, it's way easier to test this site if you turn off cookies in your Fennec.
Comment 9•13 years ago
|
||
This is still a blocker.
Updated•13 years ago
|
Whiteboard: [Engagement] → [Engagement] [layout]
Comment 11•13 years ago
|
||
There is a reduced testcase for this on bug 757324.
Assignee | ||
Comment 12•13 years ago
|
||
I've whittled down Martijn's testcase a little bit more.
Comment 13•13 years ago
|
||
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?
Assignee | ||
Comment 14•13 years ago
|
||
Alice, could you possibly find out when exactly the testcase of comment #12 got fixed on desktop?
Comment 15•13 years ago
|
||
(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
Assignee | ||
Comment 16•13 years ago
|
||
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.
Assignee | ||
Comment 17•13 years ago
|
||
(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.
Assignee | ||
Comment 18•13 years ago
|
||
Attachment #628605 -
Attachment is obsolete: true
Assignee | ||
Comment 19•13 years ago
|
||
That version of the testcase is still not working for me on Fennec nightly. How about you, JP?
Updated•13 years ago
|
tracking-fennec: --- → 15+
blocking-fennec1.0: + → -
Updated•13 years ago
|
blocking-fennec1.0: - → +
Comment 20•12 years ago
|
||
(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•12 years ago
|
||
Testing from nightlies proves this regressed:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c61e7c3a232aad0d8fb1bc2455487ce61dd9e182&tochange=0c7e2911be75b12aa98e83800231cecf127ec997
Currently doing a hg bisect.
Comment 23•12 years ago
|
||
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
Comment 24•12 years ago
|
||
Which I already knew, if I'd looked at comment 7....
Assignee | ||
Comment 25•12 years ago
|
||
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.
Assignee | ||
Comment 26•12 years ago
|
||
Attachment #630836 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 27•12 years ago
|
||
Comment 28•12 years ago
|
||
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+
Assignee | ||
Comment 29•12 years ago
|
||
(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•12 years ago
|
||
Does this patch affect desktop as well?
Assignee | ||
Comment 31•12 years ago
|
||
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.
Assignee | ||
Comment 32•12 years ago
|
||
Hmm, this causes the testcase for bug 728983 to fail. I should have tested that manually first. At least the test is working though!
Assignee | ||
Comment 33•12 years ago
|
||
Attachment #630861 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 34•12 years ago
|
||
That followup passes the test here and in bug 728983.
Updated•12 years ago
|
Attachment #630861 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 35•12 years ago
|
||
I folded the patches together and pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f28fd1e2d81
Comment 36•12 years ago
|
||
Roc, can you nom this for beta/aurora with a risk assessment? Also, does this impact desktop?
Assignee | ||
Comment 37•12 years ago
|
||
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?
Comment 38•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4f28fd1e2d81
(Merged by Ed Morley)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Comment 39•12 years ago
|
||
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+
Comment 40•12 years ago
|
||
(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?
tracking-firefox14:
--- → +
Keywords: qawanted
Comment 41•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/7b9ce6660818
https://hg.mozilla.org/releases/mozilla-aurora/rev/a125034a52b3
status-firefox14:
--- → fixed
status-firefox15:
--- → fixed
Comment 42•12 years ago
|
||
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
Assignee | ||
Comment 43•12 years ago
|
||
(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•12 years ago
|
||
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.
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•