User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.7) Gecko/2009021910 Firefox/3.0.7 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b4pre) Gecko/20090325 Shiretoko/3.5b4pre URL displays differently between 1.9.1/mozilla-central and 1.9.0. It looks broken in 1.9.1/mozilla-central. Will attach images of both. Reproducible: Always Steps to Reproduce: 1. Load the URL. Actual Results: Rendering looks broken. Expected Results: Same rendering as 1.9.0.

Is the problem the grey background? A regression range would be useful.

I attached a reduced testcase. The background seems to get painted incorrectly as if the iframe were not transformed. I narrowed down the range: the problem occurs in the windows 1.9.1 2009-02-26 nightly, but not in the 2009-02-25 nightly. (As an aside, I tried using a data URI for the src of the iframe, but I got an XML parse error. Are data URI's not allowed in this context?)

works: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090215 Minefield/3.2a1pre ID:20090215124130 http://hg.mozilla.org/mozilla-central/rev/405c4bcbd3b3 fails: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090216 Minefield/3.2a1pre ID:20090216042918 http://hg.mozilla.org/mozilla-central/rev/b5359ab6a52c => range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=405c4bcbd3b3&tochange=b5359ab6a52c in combination of the mentioned range from comment 6 (what would be roughly http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?startdate=2009-02-25+02%3A00&enddate=2009-02-26+05%3A00) both these ranges have three landings in common: Bug 462455 Bug 475343 Bug 476557

Looking at the bugs it doesn't seem like a video fix or a limit on the number of regions should come into play so that leaves Bug 476557. I tried to back this bug out to confirm but the trunk has moved on som much since the patch was checked in that I couldn't do that.

This patch to mozilla-central undoes the main functional changes of bug 476557 (bug 478079 has since landed on trunk that also changes stuff in the same area, so it interrelated to that bug too). This fixes the regression for me, so this would seem to confirm that this regression was caused by bug 476557.

roc do you think this regression is worse than bug 476557? Should take this patch till something better comes along?

This is definitely not worse than 476557. We should try to fix this though.

Just wanted to be clear that the patch I uploaded was only meant for diagnostic purposes, not a real fix, as I just shallowly looked at the patch for bug 476557 and undid the functional changes while keeping the API changes by updating the old code.

I can reproduce this bug on attachment 369603 [details] using Linux trunk nightly: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090330 Minefield/3.6a1pre Platform --> All/All, Version --> Trunk

data:text/html,... urls work just fine in this context, but you have to url-encode all the angle brackets. Here's a single-file test case, also with more verbiage stripped. And I took out the rotation because that makes it easier to do a reftest-style reference using <svg:rect> -- I was getting pixel differences at the edges of the rotated square.

Here's the reference rendering. With the bug in effect, attachment 370461 [details] draws two blue squares and this draws only one.

Data point: the extra blue square is being drawn by the normal paint path, i.e. this call at line 5564 of nsPresShell.cpp: nsLayoutUtils::PaintFrame(aRenderingContext, frame, aDirtyRegion, bgcolor); if I change 'bgcolor' to NS_RGB(255,255,255) the extra blue square goes away. Wild speculation: the display list optimizer doesn't understand SVG transforms applied to iframes (perhaps inner widgets generally?), so it's leaving a hole in the display list where it thinks the inner iframe is going to be.

Hmm, we shouldn't be getting a PresShell::Paint where 'frame' is the IFRAME's root frame. 'frame' should always be the outermost content document.

Is there an easy way to tell from inside the debugger whether or not 'frame' is the outermost content document? The effect does seem to be specific to IFRAMEs - a div with overflow:auto doesn't do it, and if there's two IFRAMEs inside the foreignObject with two different background colors on their inner documents, I get two different stripes of color in the wrong place!

Here's yet another test case, which (with the bug present) will show a green square and then two stripes to its left, one purple, one orange. (Tangentially, I would love to know why I need "iframe { margin-bottom: -4px }" in there in order for there not to be stripes of red inside the green square.) I instrumented PresShell::Paint to report the background color it computes, the frame it's called on, and the "root frame" it uses to determine the background color. On this test case, it's called nine(!) times in a row, with these unique combinations of the above three values: bgcolor=255.255.255.255 frame=0x7fb0e8b7c7f8 rootframe=0x7fb0e4959df0 bgcolor=128.0.128.255 frame=0x7fb0e8b7c7f8 rootframe=0x7fb0e3a4ff28 bgcolor=255.165.0.255 frame=0x7fb0e8b7c7f8 rootframe=0x7fb0e3a79f68 -- so it *is* always called with the same frame (what I assume is the outermost document) but FrameConstructor()->GetRootElementStyleFrame() sometimes returns something associated with that document and sometimes returns something associated with the nested documents. I seem to recall one of the bugs in this long chain being about how we actually needed to set the 'backstop color' for nested iframes equal to the background color for the inner document, so I'm continuing with the notion that PresShell::Paint is fine and this is a display list problem.

Starting at what roc said in comment 17, with framesets or iframes it appears that PresShell::Paint is not always called on the root content document (even in cases where this bug does not occur). This would produce a 'mismatch' where the PresShell of the frame of aView would not be |this|. So I tried detecting this and calling Paint on the PresShell of the frame of aView and it fixed this bug and seems to work, although I just tried a few pages with frames/iframes and without.

The problem also occurs while using moz-transform (without any SVG). That might be helpful.

(In reply to comment #18) > Is there an easy way to tell from inside the debugger whether or not 'frame' is > the outermost content document? p nsIFrameDebug::DumpFrameTree(frame)

(In reply to comment #20) > Created an attachment (id=370584) [details] > patch that makes sure PresShell::Paint is only called on a view that it owns > > Starting at what roc said in comment 17, with framesets or iframes it appears > that PresShell::Paint is not always called on the root content document (even > in cases where this bug does not occur). This would produce a 'mismatch' where > the PresShell of the frame of aView would not be |this|. So I tried detecting > this and calling Paint on the PresShell of the frame of aView and it fixed this > bug and seems to work, although I just tried a few pages with frames/iframes > and without. OK, so the question is, how are we getting into a state where we're calling PresShell::Paint on the wrong presshell?

Here is the top of a stacktrace of when PresShell::Paint gets called with the wrong PresShell. #0 PresShell::Paint (this=0xaf22b800, aView=0xb16f1a60, aRenderingContext=0xaf13f540, aDirtyRegion=@0xbfbed86c) at /mozilla/src/layout/base/nsPresShell.cpp:5543 #1 0xb344a333 in nsViewManager::RenderViews (this=0xaf246940, aView=0xaf280280, aRC=@0xaf13f540, aRegion=@0xbfbed930) at /mozilla/src/view/src/nsViewManager.cpp:607 #2 0xb344ad72 in nsViewManager::Refresh (this=0xaf246940, aView=0xaf280280, aContext=0xaf13f540, aRegion=0xaf13f2c0, aUpdateFlags=1) at /mozilla/src/view/src/nsViewManager.cpp:511 #3 0xb344b7e7 in nsViewManager::DispatchEvent (this=0xaf246940, aEvent=0xbfbedbbc, aStatus=0xbfbedad0) at /mozilla/src/view/src/nsViewManager.cpp:1103 #4 0xb34441a5 in HandleEvent (aEvent=0xbfbedbbc) at /mozilla/src/view/src/nsView.cpp:167 #5 0xb5008064 in nsWindow::DispatchEvent (this=0xb16b5560, aEvent=0xbfbedbbc, aStatus=@0xbfbedcd0) at /mozilla/src/widget/src/gtk2/nsWindow.cpp:580 #6 0xb5005b93 in nsWindow::OnExposeEvent (this=0xb16b5560, aWidget=0xb3889600, aEvent=0xbfbee114) at /mozilla/src/widget/src/gtk2/nsWindow.cpp:2302 I created the patch to detect when this is happening in nsViewManager::RenderViews, which appears to me to be the correct place to catch this.

Comment on attachment 370791 [details] [diff] [review] potential fix Superb!

Zack, can you roll up your test into a landable patch and attach it? Thanks hkygrts! (however you pronounce that...)

First thing tomorrow morning! (It's quite late here)

With this patch only the root document's PresShell's mCanvasBackgroundColor will get set because mCanvasBackgroundColor only gets set to a new value in nsPresShell::Paint and that only gets called on the root document PresShell. So I think this may regress the 'don't show a flash of white when navigating' for frames/iframes. Perhaps we should also set mCanvasBackgroundColor in nsCSSRendering::PaintBackground or somewhere else?

Frames and iframes are transparent by default. I'm not sure how much it matters that their background color persist across document loads. Maybe there are sites where it matters...

(In reply to comment #30) > Frames and iframes are transparent by default. I'm not sure how much it matters > that their background color persist across document loads. Maybe there are > sites where it matters... I haven't read it in detail, but bug 75591 seems to be about this same issue.

Should probably hold off on the checkin, I get a bunch of reftest failures with this patch. REFTEST TEST-UNEXPECTED-FAIL | /src/modules/libpr0n/test/reftest/colordepth.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/box-shadow/boxshadow-inner-basic.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/234964-1.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/308406-1.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/308406-2.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/360757-1b.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/362901-1.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/367247-s-hidden.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/372037-1.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/374719-1.xul | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/379349-1b.xhtml | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/379349-1c.xhtml | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/386401-3.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/386470-1a.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/386470-1c.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/402567-1.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/402567-2.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/410621-1.html | REFTEST TEST-UNEXPECTED-PASS | /src/layout/reftests/bugs/424074-1-ref2.xul | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/430412-1.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/456219-1a.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/456219-1b.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/456219-1c.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/bugs/456484-1.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/percent-overflow-sizing/simpleHeight100.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/percent-overflow-sizing/simpleAbsHeight.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/percent-overflow-sizing/simpleHeight100D.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/percent-overflow-sizing/simpleAbsHeightD.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/percent-overflow-sizing/simpleMinHeight100D.html | REFTEST TEST-UNEXPECTED-FAIL | /src/layout/reftests/percent-overflow-sizing/simpleAbsMinHeightD.html | REFTEST TEST-UNEXPECTED-FAIL | /src/modules/plugin/test/reftest/plugin-sanity.html | REFTEST TEST-UNEXPECTED-FAIL | /src/modules/plugin/test/reftest/plugin-alpha-zindex.html |

I think all the reftest failures are caused by one issue (except for the colordepth one, that was because the color depth of the screen was set to 16). On some elements a phantom horizontal scroll bar is drawn. Frequently this happens when the element has overflow: auto, but there are other cases. So it seems some code is being lazy and drawing an unneeded scrollbar and then depending on the behaviour of nsViewManager to clip this scrollbar drawing. In some cases the phantom scrollbar is even drawn in the status bar. (I removed [needs landing] and checkin-needed since this regression is worse then what it fixes.)

Scratch that, the reftest failures are present in a clean trunk build (bug 486065 was already filed on this issue), so they were not caused by this patch.

http://hg.mozilla.org/mozilla-central/rev/665b0bccbcf1 Thanks!!! We'll want to take this to fix the regression in 1.9.1, I guess.

(In reply to comment #30) > Frames and iframes are transparent by default. I'm not sure how much it matters > that their background color persist across document loads. Maybe there are > sites where it matters... I wanted to see how real this effect was so I tried it out and it is definitely noticeable and a regression (if you want to call it that). What would be the downside of fixing it?

No downside except it's a bit tricky to fix. Perhaps I should back this patch out, I didn't think about that issue enough. We have to do two things: set mCanvasBackgroundColor for non-root PresShells, and use it to draw at the right time. Probably this code: nsIFrame* rootFrame = FrameConstructor()->GetRootElementStyleFrame(); if (rootFrame) { const nsStyleBackground* bgStyle = nsCSSRendering::FindRootFrameBackground(rootFrame); mCanvasBackgroundColor = bgStyle->mBackgroundColor; } should be factored out and called by nsDisplayList::EnterPresShell so that every presshell we render gets its mCanvasBackgroundColor updated. And in EnterPresShell, if GetRootElementStyleFrame returns null, then we can add a display list item to paint mCanvasBackgroundColor over aReferenceFrame's bounds, since nothing else is going to paint a canvas background.

Comment on attachment 370814 [details] [diff] [review] above patch plus reftests [Checkin: Comment 38] a1.9.1=dbaron

tn: do you have a test case for the effect you describe in comment 39?

I don't want to land this on 1.9.1 until the problem in comment #40 is fixed.

(In reply to comment #42) > tn: do you have a test case for the effect you describe in comment 39? I've just been using drobo.com in a frameset with a small dummy frame on its left. Clicking links in the drobo frame is enough for my old computer to see a flash of white.

(In reply to comment #44) > > tn: do you have a test case for the effect you describe in comment 39? > I've just been using drobo.com in a frameset [snip] tn: could you attach your testcase for this?

Thanks for laying this out in detail Robert. I implemented it but there were some problems and I haven't got it to fix the flash of white yet. (In reply to comment #40) > And in EnterPresShell, if GetRootElementStyleFrame returns null, then we can > add a display list item to paint mCanvasBackgroundColor over aReferenceFrame's > bounds, since nothing else is going to paint a canvas background. GetRootElementStyleFrame never returns null inside EnterPresShell in my testing. This makes sense since EnterPresShell is called with a frame so we already have a frame tree. So I tried adding a solid color item to the bottom of the display list every time EnterPresShell is called. That didn't work either. Here's the patch for what I currently have (including printf's).

OK, I guess the problem is that nsSubDocumentFrame::BuildDisplayList is bailing out because there's no frame for the inner document. Maybe if we call GetDocShell(), then docshell->GetPresShell(), then fill the subdocument frame's content-rect with the presshell background color if it has no frame?

Thanks for laying the path forward Robert. That fixed the 'drobo.com in a frame' test case. But in further testing I found that I still get a flash of white when submitting a form in a frame/iframe (I checked that this is a regression). I will attach a testcase.

+ nsIFrame* referenceFrame = aBuilder->ReferenceFrame(); + if (!referenceFrame) + return NS_OK; + if (!referenceFrame->HasView()) + return NS_OK; You don't need this code, right? Your form testcase should be debuggable, I hope...

The form problem has nothing to do with forms, it happens anytime you navigate to a page where the background color is not defined in an external CSS file. Setting the background color of the destination page using <body bgcolor=...> or <body style="background-color: ..."> or <style>body { background-color: ...; }</style> will have a flash of white. But if you put that same style information in a separate file and link to it, no flash of white. How the body background color is specified in the initial document does not make a difference. Peering into this with the debugger is giving me some confusing results. The no flash of white case showed everything to be working as expected, hitting the no frame tree case in nsSubDocumentFrame::BuildDisplayList on the first draw after clicking the link. But when the in the case where a flash of white happens, the first draw after clicking the link, nsSubDocumentFrame::BuildDisplayList finds a frame tree. This frame tree appears to be the frame tree of the destination page. (I believe it is the frame tree of the destination page because the memory location for the body frame was the same as in a frame tree I printed later when I was sure it was drawing the destination page.) This frame tree was not completely loaded as it was missing at least the text elements. This frame tree had all of its mRects as (0,0,0,0), so it drew nothing.

I think we need to keep doing what nsViewManager::DispatchEvent was doing, and check the presshell's view manager's IsRefreshEnabled (which will need to be exposed through nsIViewManager) and paint the default background if it isn't enabled.

Checking if refresh is enabled on mInnerView and subdocView shows them both to be enabled when we get the frame tree with all (0,0,0,0) mRects in nsSubDocumentFrame::BuildDisplayList. However checking if the mRect of f is empty and drawing the default background in that case fixes the flash of white. I don't know if this is a good fix.

I'm stumped. I'd better dig into it myself instead of wasting your time.

Actually, I think I've figured it out. If we weren't in a frameset/iframe and got the (0,0,0,0) mRect case we have the default solid color item inserted by nsLayoutUtils::PaintFrame which is what would get drawn. So we just need to do the same thing in nsSubDocumentFrame::BuildDisplayList with a little extra logic to figure out when we want add a solid color item and when we want to leave it and fall back to what is drawn by the parent document.

I implemented the idea in comment 56 and it worked. This patch still needs much cleaning up.

The more I think about it, the more I think checking in nsSubDocumentFrame::BuildDisplayList if the mRect of the frame f is empty is the right fix (as I mentioned in comment 54 this does fix the problem for me). My hypothesis is that the subdocument's frame tree is not ready for a paint, but a paint event is called on its parent document; normally a paint would not be called on a frame tree with all (0,0,0,0) mRects. So we detect this and skip painting of the subdocument. When we were calling Paint on non-root documents, I think the fact that solidcolor items just paint the dirty rect with no clipping to their underlying frame; and the dirty rect being set in the view code based on view dimensions (and not frame dimensions) is what saved us from the flash of white.

How about checking IsPaintingSuppressed on the subdocument's presshell?

IsPaintingSuppressed did the trick! Unfortunately in testing that I found another problem. For transformed iframes the solidcolor item drawn to avoid the flash of white is drawing the wrong place/size. For a rotated iframe it seems to draw a (non-rotated) bounding box for the iframe. For a simple translated iframe it seems to draw untranslated. It's probably something to do with using a view instead of a frame to compute position/size. I'm investigating.

I fixed up the stuff I mentioned with transformed iframes. This patch still has the refresh enabled check, I'll look into the semantics of that and see if that is ever a possibility. I'll create a 1.9.1 patch after getting feedback about this one. I still think we aren't quite doing the right thing when painting transformed iframes, but it is not a regression. For example, if you translate right an iframe by about half its width and scroll it, the left and right halves will be out of sync for a split second and then drawn correctly. And a similar thing happens when we draw a solidcolor item, first the left half will be cleared, and then the right half. Like we are drawing the iframe as if it were untransformed (but clipped to where it actually is) and then drawing where it actually is.

(In reply to comment #61) > I still think we aren't quite doing the right thing when painting transformed > iframes, but it is not a regression. For example, if you translate right an > iframe by about half its width and scroll it, the left and right halves will be > out of sync for a split second and then drawn correctly. And a similar thing > happens when we draw a solidcolor item, first the left half will be cleared, > and then the right half. Like we are drawing the iframe as if it were > untransformed (but clipped to where it actually is) and then drawing where it > actually is. That is because the iframe has a native widget whose position is not transformed. That widget gets painted at a slightly different time from the rest of the window. The solution is just to get rid of the widget --- I'm working on it!

+ nsIFrame* referenceFrame = aBuilder->ReferenceFrame(); + if (!referenceFrame) + return nsRect(); referenceFrame can't be null, don't check for that. + if (!mView) + return nsRect(); Just use NS_ASSERTION(mView, ...) because it's a bug if both mFrame and mView are null. + nsIView* referenceView = referenceFrame->GetView(); + if (!referenceView) + return nsRect(); This *could* happen so you shouldn't rely on referenceView not being null. Actually, nsDisplaySolidColor should probably just store its bounds (relative to the reference frame). Then you don't need to store a view here and you don't need to worry about referenceFrame->GetView() being null. Just get the subdocView's rectangle, add its offset to the subdoc frame's view, then add the offset from the subdoc frame to the reference frame. You shouldn't need to add an nsDisplayClip there either. + NS_IMETHOD IsRefreshEnabled(PRBool *aRefresh) = 0; You can just make it "virtual PRBool IsRefreshEnabled() = 0;". The out-param style is legacy stuff that we're trying to get rid of. Can we get away without calling IsRefreshEnabled though? I think we should be able to.

(In reply to comment #63) > Can we get away without calling IsRefreshEnabled though? I think we should be > able to. Yeah, it never came up, I just wanted to look at the details in the view code to be sure. Refresh disabled/enabled is a property of the whole view manager hierarchy, so we'll never see refresh disabled in this case. Do I need to bump the IID of nsIPresShell?

Yes, you need to rev the nsIPresShell IID. + // If we have a frame, use that for the bounds. + if (mFrame) + return nsDisplayItem::GetBounds(aBuilder); Let's get rid of the use of mFrame here entirely. We can always use the rect path, that will be simpler IMHO. + nsDisplaySolidColor(nsRect aBounds, nscolor aColor) const nsRect& + nsRect mBounds; + nscolor mColor; Fix indent + nsIFrameDebug* fDebug = nsnull; + if (f) + fDebug = do_QueryFrame(f); Shouldn't be needed, do_QueryFrame should be null-safe + if (subdocView) + f = static_cast<nsIFrame*>(subdocView->GetClientData()); + else return NS_OK; {} around these statements + if (f) + f->PresContext()->PresShell()->IsPaintingSuppressed(&suppressed); And this one + PRBool suppressed = PR_FALSE; + if (f) + f->PresContext()->PresShell()->IsPaintingSuppressed(&suppressed); Why not make suppressed default to PR_TRUE when there's no frame, then + if (!f || suppressed) { can just test suppressed. + if (!mFrameLoader) + return NS_OK; + nsCOMPtr<nsIDocShell> docShell; + mFrameLoader->GetDocShell(getter_AddRefs(docShell)); + if (!docShell) + return NS_OK; + nsCOMPtr<nsIPresShell> presShell; + docShell->GetPresShell(getter_AddRefs(presShell)); + if (!presShell) + return NS_OK; Actually, why not just forget f, always get the presShell from the docshell this way, and call IsPaintingSuppressed on it? + nsRect viewBounds = subdocView->GetBounds(); + viewBounds.x = viewBounds.y = 0; + viewBounds = viewBounds + subdocView->GetOffsetTo(GetView()) + + GetOffsetTo(aBuilder->ReferenceFrame()); Call it shellBounds since that's what it's going to be. Setting viewBounds.x/y to zero is wrong; subtract subdocView->GetPosition() instead to get the bounds relative to subdocView (they can extent above and to the left of its "origin"!)

(In reply to comment #66) > + // If we have a frame, use that for the bounds. > + if (mFrame) > + return nsDisplayItem::GetBounds(aBuilder); > > Let's get rid of the use of mFrame here entirely. We can always use the rect > path, that will be simpler IMHO. Did you mean just in this specific case, or remove mFrame from the nsDisplaySolidColor class and just base it on a rect? > + if (!mFrameLoader) > + return NS_OK; > + nsCOMPtr<nsIDocShell> docShell; > + mFrameLoader->GetDocShell(getter_AddRefs(docShell)); > + if (!docShell) > + return NS_OK; > + nsCOMPtr<nsIPresShell> presShell; > + docShell->GetPresShell(getter_AddRefs(presShell)); > + if (!presShell) > + return NS_OK; > > Actually, why not just forget f, always get the presShell from the docshell > this way, and call IsPaintingSuppressed on it? We can't do this, sometimes painting is not suppressed but f is null.

(In reply to comment #67) > (In reply to comment #66) > > + // If we have a frame, use that for the bounds. > > + if (mFrame) > > + return nsDisplayItem::GetBounds(aBuilder); > > > > Let's get rid of the use of mFrame here entirely. We can always use the rect > > path, that will be simpler IMHO. > > Did you mean just in this specific case, or remove mFrame from the > nsDisplaySolidColor class and just base it on a rect? The latter. > > + if (!mFrameLoader) > > + return NS_OK; > > + nsCOMPtr<nsIDocShell> docShell; > > + mFrameLoader->GetDocShell(getter_AddRefs(docShell)); > > + if (!docShell) > > + return NS_OK; > > + nsCOMPtr<nsIPresShell> presShell; > > + docShell->GetPresShell(getter_AddRefs(presShell)); > > + if (!presShell) > > + return NS_OK; > > > > Actually, why not just forget f, always get the presShell from the docshell > > this way, and call IsPaintingSuppressed on it? > > We can't do this, sometimes painting is not suppressed but f is null. OK.

Comment on attachment 372805 [details] [diff] [review] trunk patch with further comments addressed + nsRect shellBounds = subdocView->GetBounds() - subdocView->GetPosition(); + shellBounds = shellBounds + subdocView->GetOffsetTo(GetView()) + + GetOffsetTo(aBuilder->ReferenceFrame()); This can be simplified a tiny bit. We know that subdocView's parent is mInnerView, and we know that mInnerView's parent is GetView(), so we can write nsRect shellBounds = subdocView->GetBounds() + mInnerView->GetPosition() + GetOffsetTo(aBuilder->ReferenceFrame()); great work!

The trunk patch doesn't apply anymore: patching file layout/base/nsCSSRendering.cpp Hunk #1 FAILED at 976 1 out of 1 hunk FAILED -- saving rejects to file layout/base/nsCSSRendering.cpp. rej patching file layout/base/nsDisplayList.cpp Hunk #1 FAILED at 165 Hunk #2 FAILED at 186 Hunk #3 FAILED at 248 Hunk #4 FAILED at 483 4 out of 4 hunks FAILED -- saving rejects to file layout/base/nsDisplayList.cpp. rej patching file layout/base/nsDisplayList.h Hunk #1 FAILED at 999 1 out of 1 hunk FAILED -- saving rejects to file layout/base/nsDisplayList.h.rej patching file layout/base/nsIPresShell.h Hunk #1 FAILED at 95 Hunk #2 FAILED at 787 2 out of 2 hunks FAILED -- saving rejects to file layout/base/nsIPresShell.h.rej patching file layout/base/nsLayoutUtils.cpp Hunk #1 FAILED at 932 Hunk #2 FAILED at 1101 2 out of 2 hunks FAILED -- saving rejects to file layout/base/nsLayoutUtils.cpp. rej patching file layout/base/nsPresShell.cpp Hunk #1 FAILED at 1124 Hunk #2 FAILED at 5513 2 out of 2 hunks FAILED -- saving rejects to file layout/base/nsPresShell.cpp.re j patching file layout/generic/nsFrameFrame.cpp Hunk #1 FAILED at 315 1 out of 1 hunk FAILED -- saving rejects to file layout/generic/nsFrameFrame.cpp .rej patch failed, unable to continue (try -v)

Landed and backed out: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1240038966.1240042788.19986.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1240038966.1240044384.22288.gz

The tests that failed or timed out in those logs all run fine on my linux box. Specifically these tests /tests/content/base/test/test_bug333198.html /tests/content/events/test/test_bug402089.html chrome://mochikit/content/chrome/content/xul/content/test/test_bug398289.html

Maybe try running them under valgrind? (Running mochitests under valgrind is a little tricky; I tend to modify runtests.py so that still creates the profile and then stops, and then run firefox manually on the profile it created.)

(In reply to comment #77) > Maybe try running them under valgrind? (Running mochitests under valgrind is a > little tricky; I tend to modify runtests.py so that still creates the profile > and then stops, and then run firefox manually on the profile it created.) Thanks for the tip. While looking at runtests.py I found that it appeared to have support for valgrind, so I was able to just do python runtests.py --test-path=... --debugger=valgrind from <obj-dir>/_tests/testing/mochitest. However, when doing this, even on a clean trunk build, the browser crashes. (I tried it on a few different random tests, so I don't think it is test specific.) I can run firefox normally (not mochitests) with valgrind with no problem. Here is the end of the output from a single test mochitest run. TEST-UNEXPECTED-FAIL | (automation.py) | Exited with code 1 during test run INFO | (automation.py) | Application ran for: 0:10:30.403055 TEST-UNEXPECTED-FAIL | (automation.py) | Browser crashed (minidump found) WARNING: nsExceptionService ignoring thread destruction after shutdown: file /src/xpcom/base/nsExceptionService.cpp, line 194 nsStringStats => mAllocCount: 18293 => mReallocCount: 281 => mFreeCount: 18293 => mShareCount: 2401 => mAdoptCount: 238 => mAdoptFreeCount: 238 TEST-UNEXPECTED-FAIL | runtests-leaks | missing output line for total leaks!

I was briefly able to reproduce the timeout on the test for bug 402089, but only when running it as part of all the tests in its directory. I think this is because when you run more than one test it runs them in an iframe. After recompiling, it started passing again though. So no luck there. The test for bug 398289 is basically a reftest, comparing a before and after screenshot, and handily its failing message provided data URIs for expected/got PNGs. This showed that a plain background was still drawn when onload fires. So my working hypothesis right now is that we are using the draw background only path for iframes when normally we would be building the display list for the frame tree. This is either because painting is suppressed for any paints before onload fires (seems unlikely) or the method of getting the presshell from the docshell is messing things up. Specifically nsFrameLoader::GetDocShell isn't a pure getter as it calls EnsureDocShell to create a docshell if one doesn't exist. EnsureDocShell has wonderful comments like "This is kinda whacky", "This is nasty", and "this is such a total hack.... We really need to have a better setup for doing this", so perhaps it is not what we want to call here.

DocumentViewerImpl::LoadComplete fires the load event before we unsuppress painting (via mPresShell->UnsuppressPainting()). So it's indeed possible for painting to still be suppressed when the load handlers run, which explains how these test failures could be caused by your patch. So there are two options: 1) unsuppress painting in LoadComplete before we dispatch the load event. 2) Modify your patch as follows: + if (!f || suppressed) { + // If we don't have a frame or painting of the PresShell is suppressed, + // try to draw the default background color. (Bug 485275) + + // Get the bounds of subdocView relative to the reference frame. + nsRect shellBounds = subdocView->GetBounds() + + mInnerView->GetPosition() + + GetOffsetTo(aBuilder->ReferenceFrame()); + rv = aLists.Content()->AppendNewToBottom( + new (aBuilder) nsDisplaySolidColor( + shellBounds, + presShell->GetCanvasBackground())); + return rv; Instead of returning, if f is non-null we could just continue and draw the frame tree on top of the solid color. That would be bad when the background is partially translucent but you've already filed that bug about for toplevel windows and it doesn't matter all that much anyway.

I prefer option 2 because it's a lot more like the current behaviour than option 1, so safer for the 1.9.1 branch.

(In reply to comment #80) > Instead of returning, if f is non-null we could just continue and draw the > frame tree on top of the solid color. That would be bad when the background is > partially translucent but you've already filed that bug about for toplevel > windows and it doesn't matter all that much anyway. While for top level windows it doesn't matter as much (the page is asking for trouble if it's uses partial transparency on something which has nothing behind it). But with iframes they will have something behind them, so it should have a well defined behavior if we are going to support it. (Granted, we would only be drawing the wrong colors while in transition between pages.) If you're ok with this, I'm ok with this (at least for 1.9.1). So here is a patch doing option 2. I also added another change -- we shouldn't bother with the solid color stuff if the display list builder is for event delivery. The other failures I think are due to events firing at onload getting lost because they hit a solid color item and die there because solid color items have no frames. Even if this is not the cause I think it is still the right thing to do.

Comment on attachment 373799 [details] [diff] [review] m-c patch v4 Let's do it!

Pushed to try servers ... should show up here under rocallahan@mozilla.com, eventually http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry

We probably should be able to fix the other bug you filed and avoid setting the nsDisplaySolidColor when the window has got its own background, too.

Try servers look good. Let's reland.

OS X now has one mochitest failure.

I'm very confident that's not related.

minor comment fix

http://hg.mozilla.org/mozilla-central/rev/444eff22b675 Thanks tn! Let's wait for a week or so to make sure there's no more fallout, then put something together for 1.9.1.

Now the fallout looks to have been contained, we should fix this regression on 1.9.1.

Asking for review since this wasn't a 100% dead simple port. Removing checkin-needed as I believe this needs approval1.9.1 first (the earlier patch with approval1.9.1 is a subset of this patch).

Roadmap to this patch and followups. Bug 485275, bug 490376, and bug 492014 should be applied in that order. Bug 491848 is independent.

Thanks a ton!

Comment on attachment 377786 [details] [diff] [review] 1.9.1 combined patch v4a [Checkin: Comment 95] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4e51f2008cbc after fixing context for { patching file layout/reftests/bugs/reftest.list Hunk #1 FAILED at 1023 1 out of 1 hunks FAILED }

verified FIXED on builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090527 Minefield/3.6a1pre ID:20090527031500 and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090527 Shiretoko/3.5pre ID:20090527031214