406784 - Backgrounds are no longer painting at all before pageload

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P3)

Description

[Stuart Morgan]

A fairly recent build with the patch from bug 401328 applied locally was correctly painting new tabs white, but in today's official build that code is never getting called at all, and the background is the light-grey window background. Something else appears to have regressed recently.

ChildView is getting the drawRect: event, but the dispatch isn't getting to nsWebBrowser. I haven't had a chance to investigate more than that yet.

Comment 1

[Stuart Morgan]

The white square of bug 401328 appears in the 12/1 nightly, but not the 12/2, so that looks like the regression range for this issue.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=Camino&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-12-01+01%3A00&maxdate=2007-12-02+01%3A00&cvsroot=%2Fcvsroot

The xpt audit, perhaps?  fantasai's patch?  Most everything gfx/layout-related in there got backed out due to test failures.

Comment 3

[Stuart Morgan]

It's bug 405732; the setup change in that patch appears to have changed the target of the mEventCallback at
http://mxr.mozilla.org/mozilla/source/widget/src/cocoa/nsChildView.mm#1347
so that it's no longer going to nsWebBrowser and hitting the just-fixed drawing code there (bug 401328)

Any thoughts on how to get back to a state where we draw a background here?

Comment 4

[Boris Zbarsky [:bzbarsky]]

So we're hitting the case when mPresShell is not null in SetDOMDocument?  Or is it one of the other changes?  If so, which one?

Comment 5

[Stuart Morgan]

Yes, it's the new MakeWindow at the end of the |if (mPresShell)| block; commenting out just that line fixes this.

I'm not sure if it's related or not, but I noticed that with or without the change this is being logged after I begin the action of opening the tab but before getting to the SetDOMDocument:
  WARNING: NS_ENSURE_TRUE(presShell) failed: file /Volumes/CaminoTrunk/mozilla/dom/src/base/nsGlobalWindow.cpp, line 6208

Comment 6

[philippe (part-time)]

On 10.4.11 PPC builds, and this applies to *both* Camino and Minefield, the 'tab bleeding' is back (bug 362549). And I can't reproduce what Stuart sees.
Using hourly builds, I found that it regressed between:
20071201_0239 Minefield/3.0b2pre works
20071201_0310 Minefield/3.0b2pre fails

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1196505540&maxdate=1196507399

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Yes, it's the new MakeWindow at the end of the |if (mPresShell)| block;

What's the stack to hitting this code?  Note that before bug 405732 hitting that code led to crashes, so I'm a little surprised we're hitting that code regularly in Camino...

Comment 8

[Stuart Morgan]

(In reply to comment #7)
> What's the stack to hitting this code?

#0  DocumentViewerImpl::SetDOMDocument (this=0x1fcb8290, aDocument=0x1058698) at /Volumes/CaminoTrunk/mozilla/layout/base/nsDocumentViewer.cpp:1623
#1  0x135941e6 in nsDocShell::CreateAboutBlankContentViewer (this=0x1fca55b0, aPrincipal=0x0) at /Volumes/CaminoTrunk/mozilla/docshell/base/nsDocShell.cpp:5209
#2  0x13594382 in nsDocShell::EnsureContentViewer (this=0x1fca55b0) at /Volumes/CaminoTrunk/mozilla/docshell/base/nsDocShell.cpp:5098
#3  0x1359e066 in nsDocShell::GetInterface (this=0x1fca55b0, aIID=@0x1415ca50, aSink=0xbfffe30c) at /Volumes/CaminoTrunk/mozilla/docshell/base/nsDocShell.cpp:454
#4  0x135bf112 in nsWebShell::GetInterface (this=0x1fca55b0, aIID=@0x1415ca50, aInstancePtr=0xbfffe30c) at /Volumes/CaminoTrunk/mozilla/docshell/base/nsWebShell.cpp:635
#5  0x002ba151 in nsGetInterface::operator() (this=0xbfffe390, aIID=<incomplete type>, aInstancePtr=<incomplete type>) at nsIInterfaceRequestorUtils.cpp:52
#6  0x13d60d6b in nsCOMPtr<nsIDOMDocument>::assign_from_helper (this=0xbfffe38c, helper=@0xbfffe390, aIID=@0x1415ca50) at nsCOMPtr.h:1335
#7  0x13d60f0a in nsCOMPtr<nsIDOMDocument>::nsCOMPtr (this=0xbfffe38c, helper=@0xbfffe390) at nsCOMPtr.h:694
#8  0x13d60f34 in nsCOMPtr<nsIDOMDocument>::nsCOMPtr (this=0xbfffe38c, helper=@0xbfffe390) at nsCOMPtr.h:695
#9  0x13e32123 in nsGlobalWindow::GetDocument (this=0x1fcb4990, aDocument=0xbfffe3dc) at /Volumes/CaminoTrunk/mozilla/dom/src/base/nsGlobalWindow.cpp:2319
#10 0x13bbd09e in nsDataDocumentContentPolicy::ShouldLoad (this=0x19c90400, aContentType=6, aContentLocation=0x1fc910b0, aRequestingLocation=0x0, aRequestingContext=0x1fcb49bc, aMimeGuess=@0x396944, aExtra=0x0, aDecision=0xbfffe83a) at /Volumes/CaminoTrunk/mozilla/content/base/src/nsDataDocumentContentPolicy.cpp:71
#11 0x13b9ecc5 in nsContentPolicy::CheckPolicy (this=0x19c90170, policyMethod={__pfn = 0xd, __delta = 0}, contentType=6, contentLocation=0x1fc910b0, requestingLocation=0x0, requestingContext=0x1fcb49bc, mimeType=@0x396944, extra=0x0, decision=0xbfffe83a) at /Volumes/CaminoTrunk/mozilla/content/base/src/nsContentPolicy.cpp:157
#12 0x13b9de6b in nsContentPolicy::ShouldLoad (this=0x19c90170, contentType=6, contentLocation=0x1fc910b0, requestingLocation=0x0, requestingContext=0x1fcb49bc, mimeType=@0x396944, extra=0x0, decision=0xbfffe83a) at /Volumes/CaminoTrunk/mozilla/content/base/src/nsContentPolicy.cpp:218
#13 0x135b8282 in NS_CheckContentLoadPolicy (contentType=6, contentLocation=0x1fc910b0, originPrincipal=0x0, context=0x1fcb49bc, mimeType=@0x396944, extra=0x0, decision=0xbfffe83a, policyService=0x0, aSecMan=0x0) at nsContentPolicyUtils.h:223
#14 0x135a11c9 in nsDocShell::InternalLoad (this=0x1fca55b0, aURI=0x1fc910b0, aReferrer=0x0, aOwner=0x0, aFlags=1, aWindowTarget=0x1fcae820, aTypeHint=0x0, aPostData=0x0, aHeadersData=0x0, aLoadType=1, aSHEntry=0x0, aFirstParty=1, aDocShell=0x0, aRequest=0x0) at /Volumes/CaminoTrunk/mozilla/docshell/base/nsDocShell.cpp:6577
#15 0x1358e9e2 in nsDocShell::LoadURI (this=0x1fca55b0, aURI=0x1fc910b0, aLoadInfo=0x1fcae7e0, aLoadFlags=0, aFirstParty=1) at /Volumes/CaminoTrunk/mozilla/docshell/base/nsDocShell.cpp:896
#16 0x13599344 in nsDocShell::LoadURI (this=0x1fca55b0, aURI=0xbfffebe8, aLoadFlags=0, aReferringURI=0x0, aPostStream=0x0, aHeaderStream=0x0) at /Volumes/CaminoTrunk/mozilla/docshell/base/nsDocShell.cpp:2884
#17 0x169eb4b2 in nsWebBrowser::LoadURI (this=0x1fca0ca0, aURI=0xbfffebe8, aLoadFlags=0, aReferringURI=0x0, aPostDataStream=0x0, aExtraHeaderStream=0x0) at /Volumes/CaminoTrunk/mozilla/embedding/browser/webBrowser/nsWebBrowser.cpp:649
[...]

> Note that before bug 405732 hitting that code led to crashes, so I'm
> a little surprised we're hitting that code regularly in Camino...

I've been getting the impression that embedding and Firefox have fairly different handling around the setup of new windows (in the Gecko sense, not the UI sense), given the recent bugs we've been hitting.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Huh.  And at what point did mPreShell get set there?  Could you breakpoint in nsDocShell::CreateAboutBlankContentViewer and then from there see where the shell is created in the new viewer?

Comment 10

[Damon Sicore (:damons)]

+'ing.  P4.

Comment 11

[Phil Ringnalda (:philor)]

So, since apparently Firefox and Camino both developed problems with the background of loading pages at the same instant, should this move to Widget: Cocoa, or are they separate issues and we should have another bug for Firefox?

Comment 12

[Stuart Morgan]

Sorry for the delay. mPresShell is getting set up from:

#0  DocumentViewerImpl::InitPresentationStuff (this=0x1bdf68c0, aDoInitialReflow=0) at /Volumes/Chambord/Camino/trees/trunk/mozilla/layout/base/nsDocumentViewer.cpp:678
#1  0x134092ee in DocumentViewerImpl::InitInternal (this=0x1bdf68c0, aParentWidget=0x1bdf1000, aState=0x0, aDeviceContext=0x1bdf6ef0, aBounds=@0xbfffdf04, aDoCreation=1, aInPrintPreview=0, aNeedMakeCX=1) at /Volumes/Chambord/Camino/trees/trunk/mozilla/layout/base/nsDocumentViewer.cpp:909
#2  0x13409c70 in DocumentViewerImpl::Init (this=0x1bdf68c0, aParentWidget=0x1bdf1000, aDeviceContext=0x1bdf6ef0, aBounds=@0xbfffdf04) at /Volumes/Chambord/Camino/trees/trunk/mozilla/layout/base/nsDocumentViewer.cpp:658
#3  0x131d6be0 in nsDocShell::SetupNewViewer (this=0x1bdf15c0, aNewViewer=0x1bdf68c0) at /Volumes/Chambord/Camino/trees/trunk/mozilla/docshell/base/nsDocShell.cpp:6281
#4  0x131cdd28 in nsDocShell::Embed (this=0x1bdf15c0, aContentViewer=0x1bdf68c0, aCommand=0x13245c5a "", aExtraInfo=0x0) at /Volumes/Chambord/Camino/trees/trunk/mozilla/docshell/base/nsDocShell.cpp:4802
#5  0x131d4c82 in nsDocShell::CreateAboutBlankContentViewer (this=0x1bdf15c0, aPrincipal=0x0) at /Volumes/Chambord/Camino/trees/trunk/mozilla/docshell/base/nsDocShell.cpp:5209
...

So it's ultimately from the CreateAboutBlankContentViewer line right before it calls the ill-fated SetDOMDocument.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm.  We hit that code a lot.  roc, why didn't this crash before bug 405732 landed?

In any case, in this case the "new" document is the same as the existing one, so we shouldn't even be messing with the presshell, I would think...  It would still be good to figure out why messing with it breaks, and why we didn't use to get dangling views in situations like this.  Or did we?

Moving this bug to Core, where it belongs.

Comment 14

[Boris Zbarsky [:bzbarsky]]

And this should be P3, imo, if not P2.

Come to think of it, the reason for this bug is probably that we call MakeWindow but not InitPresentationStuff (like the other MakeWindow caller), so never call mViewManager->SetDefaultBackgroundColor.  It looks like we also never set the window dimensions on the viewmanager.

Comment 15

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

Attachment: call InitPresentationStuff

This what I think we should do. However, I don't know how to test this, and this is fragile stuff (to me at least).

I checked http://www.bseindia.com/ and it currently crashes, apparently for unrelated reasons :-( (filed bug 411082).

Comment 16

[Stuart Morgan]

(In reply to comment #15)
> However, I don't know how to test this

Well, it certainly seems to fix Camino at least.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 295724 [details] [diff] [review]
call InitPresentationStuff

This adds a DisableRefresh() call on the viewmanager that we didn't use to have in this method.  Where's the matching EnableRefresh()?    I guess the StartLayout call from nsXMLContentSink::OnTransformDone will handle that for you?  Please double-check that?  And perhaps document that after calling SetDOMDocument the caller needs to deal with enabling refresh on the relevant viewmanager?

r+sr=bzbarsky with that

Comment 18

[Boris Zbarsky [:bzbarsky]]

Oh, and "fragile" is a very good description of this code, sadly.

Comment 19

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

The nsXMLContentSink SetDOMDocument calls are fine, but nsDocShell::CreateAboutBlankContentViewer calls SetDOMDocument with no following EnableRefresh. But then again I don't see how the initial reflow happens for that content viewer, so I don't understand how CreateAboutBlankContentViewer is used.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Hmm.  That SetDOMDocument call is a little weird, yeah.  Part of it is that those documents are pretty much always very transient, so it might be that we in fact never did reflow for them...  Maybe we can just EnableRefresh() in CreateAboutBlankContentViewer()?  Or avoid the DisableRefresh() in the SetDOMDocument case?

Gods, I hate EnableRefresh/DisableRefresh.  :(

Comment 21

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

Adding EnableRefresh to CreateAboutBlankContentViewer seems like the cleanest thing to do but it might have side effects :-(. I think the safest thing to do is avoid the DisableRefresh in SetDOMDocument. With compositor this should become a non-issue anyway.

Comment 22

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

Attachment: fix v2

This will now do a synchronous repaint during the SetDOMDocument when a presentation already existed, without having done an initial reflow. We should at least have the view manager set up with the right size and a background color, so I hope this is OK.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 295908 [details] [diff] [review]
fix v2

Yeah, let's do this for now.

Comment 24

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

checked in