485275 - 1.9.1/mozilla-central displays svg transformed foreign object html differently then 1.9

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Timothy Nikkel (:tnikkel)]

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.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: 1.9.0 rendering

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: 1.9.1 rendering

Comment 3

[Robert Longson [:longsonr]]

Is the problem the grey background?

A regression range would be useful.

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: reduced testcase part 1 of 2

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: reduced testcase part 2 of 2

Comment 6

[Timothy Nikkel (:tnikkel)]

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?)

Comment 7

[(mostly gone) XtC4UaLL [:xtc4uall]]

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

Comment 8

[Robert Longson [:longsonr]]

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.

Comment 9

[Timothy Nikkel (:tnikkel)]

Attachment: trunk patch to revert part of bug 476557 (and bug 478079) for testing only

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.

Comment 10

[Robert Longson [:longsonr]]

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

Comment 11

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

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

Comment 12

[Timothy Nikkel (:tnikkel)]

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.

Comment 13

[Daniel Holbert [:dholbert]]

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

Comment 14

[Zack Weinberg (:zwol)]

Attachment: super minimized single file test case

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.

Comment 15

[Zack Weinberg (:zwol)]

Attachment: reference for attachment 370461

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

Comment 16

[Zack Weinberg (:zwol)]

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.

Comment 17

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

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.

Comment 18

[Zack Weinberg (:zwol)]

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!

Comment 19

[Zack Weinberg (:zwol)]

Attachment: more complicated test case showing multi-iframe effect

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.

Comment 20

[Timothy Nikkel (:tnikkel)]

Attachment: 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.

Comment 21

[Timothy Nikkel (:tnikkel)]

Attachment: testcase that uses moz-transform and not svg

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

Comment 22

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

(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)

Comment 23

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

(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?

Comment 24

[Timothy Nikkel (:tnikkel)]

Attachment: potential fix

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 25

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

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

Superb!

Comment 26

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

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

Comment 27

[Zack Weinberg (:zwol)]

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

Comment 28

[Timothy Nikkel (:tnikkel)]

Attachment: above patch plus reftests [Checkin: Comment 38]

Comment 29

[Timothy Nikkel (:tnikkel)]

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?

Comment 30

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

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...

Comment 31

[Timothy Nikkel (:tnikkel)]

(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.

Comment 32

[Timothy Nikkel (:tnikkel)]

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 |

Comment 33

[Timothy Nikkel (:tnikkel)]

Attachment: testcase for regression caused by this patch

Comment 34

[Timothy Nikkel (:tnikkel)]

Attachment: reference for attachment 370979

Comment 35

[Timothy Nikkel (:tnikkel)]

Attachment: what you actually get with this patch when viewing attachment 370979

Comment 36

[Timothy Nikkel (:tnikkel)]

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.)

Comment 37

[Timothy Nikkel (:tnikkel)]

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.

Comment 38

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

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.

Comment 39

[Timothy Nikkel (:tnikkel)]

(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?

Comment 40

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

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 41

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

Comment on attachment 370814 [details] [diff] [review]
above patch plus reftests
[Checkin: Comment 38]

a1.9.1=dbaron

Comment 42

[Zack Weinberg (:zwol)]

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

Comment 43

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

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

Comment 44

[Timothy Nikkel (:tnikkel)]

(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.

Comment 45

[Daniel Holbert [:dholbert]]

(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?

Comment 46

[Timothy Nikkel (:tnikkel)]

Attachment: WIP patch to fix regression

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).

Comment 47

[Timothy Nikkel (:tnikkel)]

Attachment: drobo.com frameset testcase for regression

Comment 48

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

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?

Comment 49

[Timothy Nikkel (:tnikkel)]

Attachment: WIP v2 patch to fix regression

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.

Comment 50

[Timothy Nikkel (:tnikkel)]

Attachment: testcase for submitting a form in an iframe

Comment 51

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

+    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...

Comment 52

[Timothy Nikkel (:tnikkel)]

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.

Comment 53

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

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.

Comment 54

[Timothy Nikkel (:tnikkel)]

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.

Comment 55

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

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

Comment 56

[Timothy Nikkel (:tnikkel)]

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.

Comment 57

[Timothy Nikkel (:tnikkel)]

Attachment: WIP v3 patch to fix regression

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

Comment 58

[Timothy Nikkel (:tnikkel)]

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.

Comment 59

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

How about checking IsPaintingSuppressed on the subdocument's presshell?

Comment 60

[Timothy Nikkel (:tnikkel)]

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.

Comment 61

[Timothy Nikkel (:tnikkel)]

Attachment: patch to fix regression

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.

Comment 62

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

(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!

Comment 63

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

+  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.

Comment 64

[Timothy Nikkel (:tnikkel)]

Attachment: above patch with comments addressed

(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?

Comment 65

[Timothy Nikkel (:tnikkel)]

Attachment: combined patch for 1.9.1

Comment 66

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

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"!)

Comment 67

[Timothy Nikkel (:tnikkel)]

(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.

Comment 68

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

(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 69

[Timothy Nikkel (:tnikkel)]

Attachment: trunk patch with further comments addressed

Comment 70

[Timothy Nikkel (:tnikkel)]

Attachment: combined patch for 1.9.1 with comments addressed

Comment 71

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

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!

Comment 72

[Zack Weinberg (:zwol)]

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)

Comment 73

[Timothy Nikkel (:tnikkel)]

Attachment: m-c patch for checkin

Comment 74

[Timothy Nikkel (:tnikkel)]

Attachment: 1.9.1 combined patch v3

Comment 75

[Dão Gottwald [:dao]]

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

Comment 76

[Timothy Nikkel (:tnikkel)]

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

Comment 77

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

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.)

Comment 78

[Timothy Nikkel (:tnikkel)]

(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!

Comment 79

[Timothy Nikkel (:tnikkel)]

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.

Comment 80

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

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.

Comment 81

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

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.

Comment 82

[Timothy Nikkel (:tnikkel)]

Attachment: m-c patch v4

(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 83

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

Comment on attachment 373799 [details] [diff] [review]
m-c patch v4

Let's do it!

Comment 84

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

Pushed to try servers ... should show up here under [e-mail], eventually
http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry

Comment 85

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

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.

Comment 86

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

Try servers look good. Let's reland.

Comment 87

[Timothy Nikkel (:tnikkel)]

OS X now has one mochitest failure.

Comment 88

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

I'm very confident that's not related.

Comment 89

[Timothy Nikkel (:tnikkel)]

Attachment: m-c patch v4a for checkin [Checkin: Comment 90]

minor comment fix

Comment 90

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

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.

Comment 91

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

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

Comment 92

[Timothy Nikkel (:tnikkel)]

Attachment: 1.9.1 combined patch v4a [Checkin: Comment 95]

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).

Comment 93

[Timothy Nikkel (:tnikkel)]

Roadmap to this patch and followups.

Bug 485275, bug 490376, and bug 492014 should be applied in that order.

Bug 491848 is independent.

Comment 94

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

Thanks a ton!

Comment 95

[Serge Gautherie (:sgautherie)]

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
}

Comment 96

[Aakash Desai [:aakashd]]

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