283147 - Crash [@ TableBackgroundPainter::TableBackgroundPainter] using vbox:hover{display:table;} with inside iframe in it

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase.

Talkback ID: TB3858077Q

TableBackgroundPainter::TableBackgroundPainter 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/tables/nsTablePainter.cpp,
line 257]
nsTableRowGroupFrame::Paint 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/tables/nsTableRowGroupFrame.cpp,
line 220]
nsBoxFrame::PaintChild 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1478]
nsBoxFrame::PaintChildren 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1604]
nsBoxFrame::Paint 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1427]
nsBoxFrame::PaintChild 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1478]
nsBoxFrame::PaintChildren 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1604]
nsBoxFrame::Paint 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1427]
nsContainerFrame::PaintChild 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/generic/nsContainerFrame.cpp,
line 305]
nsContainerFrame::PaintChildren 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/generic/nsContainerFrame.cpp,
line 229]
nsContainerFrame::Paint 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/generic/nsContainerFrame.cpp,
line 210]
PresShell::Paint 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsPresShell.cpp,
line 5473]
nsView::Paint 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/view/src/nsView.cpp, line
316]
nsViewManager::RenderDisplayListElement 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/view/src/nsViewManager.cpp,
line 1467]
nsViewManager::RenderViews 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/view/src/nsViewManager.cpp,
line 1382]
nsViewManager::Refresh 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/view/src/nsViewManager.cpp,
line 945]
nsViewManager::DispatchEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/view/src/nsViewManager.cpp,
line 2023]
HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/view/src/nsView.cpp, line
174]
nsWindow::DispatchEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 1114]
nsWindow::ProcessMessage 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 3947]
nsWindow::WindowProc 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 1400]
USER32.dll + 0x2a420 (0x77e3a420)
USER32.dll + 0x4750 (0x77e14750)
USER32.dll + 0x55b0 (0x77e155b0)
ntdll.dll + 0x1ff57 (0x77f9ff57)
USER32.dll + 0x1e6c5 (0x77e2e6c5)
nsWindow::ProcessMessage 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 4158]
nsWindow::WindowProc 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 1400]
USER32.dll + 0x2a420 (0x77e3a420)
USER32.dll + 0x4605 (0x77e14605)
USER32.dll + 0xa7ba (0x77e1a7ba)
nsAppStartup::Run 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/toolkit/components/startup/src/nsAppStartup.cpp,
line 145]
main 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/browser/app/nsBrowserApp.cpp,
line 60]
KERNEL32.DLL + 0x2893d (0x7c59893d)

Comment 1

[Martijn Wargers (dead)]

Attachment: Testcase

This testcase makes Mozilla crash when hovering over the content area.
It might be that it only crashes when you save it first and then open the
testcase.
Inside the <vbox> there is <iframe src="doesntexist.html"/> inside the
testcase.
It seems to have to point to a non-existing file on your own computer.

When the iframe points to something that exists, it doesn't crash, but seems to
reload the content inside the iframe on hovering over, which also looks like a
bug to me (because I didn't order the iframe to reload).

Comment 2

[Martijn Wargers (dead)]

Yes, the testcase should be saved first and then viewed on your own computer,
otherwise it won't crash.

Comment 3

[Martijn Wargers (dead)]

Also crashes with Mozilla1.4, so no (recent) regression.

Comment 4

[Bernd]

Attachment: minimalistic patch

Comment 5

[Bernd]

Comment on attachment 175718 [details] [diff] [review]
minimalistic patch

Boris should I add a comment that we don't bother about the result as all other
callers?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175718 [details] [diff] [review]
minimalistic patch

Sure, but why do we even get an error rv here?	Is it becase the iframe's
Init() throws?	Could you please file a followup bug on this?

Comment 7

[Bernd]

The url is not loadable
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsFrameLoader.cpp#256
fails, as a consequence we dont return a docshell
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsFrameFrame.cpp#616

showdocshell fails ....
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsFrameFrame.cpp#643

as consequence InitandRestoreFrame failes..
http://lxr.mozilla.org/seamonkey/source/layout/base/nsCSSFrameConstructor.cpp#5997

cleans up the mess and returns the anyway broken return value

Comment 8

[Boris Zbarsky [:bzbarsky]]

Hmm...  Yeah, please file a bug on that.  That seems to be wrong.  In
particular, it sounds to me like changing the "src" on a frame that started out
with an unloadable URI will not work...

Comment 9

[Bernd]

Boris: exactly my thought today morning under the shower

Comment 10

[Bernd]

Attachment: testcase

mind experiments can be sometimes wrong, the only valid is the real experiment.
That works flawless even when loaded locally

Comment 11

[Boris Zbarsky [:bzbarsky]]

Er...  So are we getting failure from Init() but ignoring it and putting the
frame in the frame tree anyway?

Comment 12

[Bernd]

Not exactly

5996 
5997       rv = InitAndRestoreFrame(aState, aContent, geometricParent,
5998                                aStyleContext, nsnull, newFrame);
5999 
6000       if (NS_FAILED(rv)) {
6001         newFrame->Destroy(aState.mPresContext);
6002         return rv;
6003       }

we clean up the mess after failing, we don't append the frame, but report we
failed to append a child to the childlist. So what we/I are missing here is a
clear for a mediocre programmer guideline how to handle this like returning
NS_OK if we cleaned up???

My usual dream of copy and paste of other places that do it correctly dies fast
at
http://lxr.mozilla.org/seamonkey/source/layout/base/nsCSSFrameConstructor.cpp#5316
and 
5333     } else {
5334       NS_ASSERTION(NS_SUCCEEDED(rv), "InitAndRestoreFrame failed");
5335       // See if we need to create a view, e.g. the frame is absolutely
5336       // positioned
5337       nsHTMLContainerFrame::CreateViewForFrame(newFrame, aParentFrame,
PR_FALSE);
I doubt that it is great idea to assert and then to create a view if failed to
initialize the frame.

Table frames then after it consider the failure critical and start that error
propagation without trying stop the desaster at the earliest moment. Blocks are
much more clever at this.
The patch here tries to block at least the entrance, this is the second time it
hit me (bug 189751). 

I guess that changing the url causes a reframe, but dont see how that works.

Comment 13

[Boris Zbarsky [:bzbarsky]]

If I load the second testcase (with the onload stuff removed from it) from local
disk and dump frames, I get:

  FrameOuter(iframe)(1)@0x85aec94 [view=0x85b8d58] {0,0,4256,2156}
    [state=00002024] [content=0x85b2868]

So we definitely have an nsSubDocumentFrame there... this is in the "failure" case.

Comment 14

[Bernd]

fix checked in, before closing we need to sort out that iframe - docshell blues

Comment 15

[Bernd]

/me rofl

Just imagine  that you call the function below twice with a nonexisting url

NS_IMETHODIMP
nsSubDocumentFrame::GetDocShell(nsIDocShell **aDocShell)
{
  *aDocShell = nsnull;

  nsIContent* content = GetContent();
  if (!content) {
    // Hmm, no content in this frame
    // that's odd, not much to be done here then.
    return NS_OK;
  }

  if (!mFrameLoader) {
    nsCOMPtr<nsIFrameLoaderOwner> loaderOwner = do_QueryInterface(content);

    if (loaderOwner) {
      loaderOwner->GetFrameLoader(getter_AddRefs(mFrameLoader));
    }

    if (!mFrameLoader) {
      // No frame loader available from the content, create our own...
      mFrameLoader = new nsFrameLoader(content);
      if (!mFrameLoader)
        return NS_ERROR_OUT_OF_MEMORY;

      // ... remember that we own this frame loader...
      mOwnsFrameLoader = PR_TRUE;

      // ... and tell it to start loading.
      nsresult rv = mFrameLoader->LoadFrame();
      NS_ENSURE_SUCCESS(rv, rv);
    }
  }

  return mFrameLoader->GetDocShell(aDocShell);
}


On the first run mFrameLoader is null we will fail to load the src but have a
mFrameLoader. On the second call we will get a docshell and return NS_OK if the
docshell exists.

The html parser calls this functions when it encounters an iframe. When the
frame constructor creates the iframe it is allready the second call to this
function.

The xul parser does not call the function before frame construction and so
during frame construction we fail to get a docshell.

So my guess is that the fix for bug 138012 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsFrameFrame.cpp&mark=617#588
is doing a little bit to much

Comment 16

[Bernd]

Please notice that in bug 138012

the change from
      mFrameLoader->LoadFrame(); 
to 
      nsresult rv = mFrameLoader->LoadFrame();
      NS_ENSURE_SUCCESS(rv, rv);

did not get reviewed, so it might be debug code that got checked in. 

So Boris, shall we backout unreviewed code? ;-)

Comment 17

[Boris Zbarsky [:bzbarsky]]

Yes.  Post a patch and I'll review?  Failing to load the URI should not throw
from EnsureDocshell and so forth.

Comment 18

[Bernd]

Attachment: local xul testcase

this testcase demonstrates the idea that early stopping with the docshell is
bad.

Comment 19

[Bernd]

Attachment: dont check return

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 176596 [details] [diff] [review]
dont check return

r+sr=bzbarsky if you add a comment that explains that failure to load a URL
does not constitute failure to create/initialize the docshell and that
therefore the LoadFrame() call's return value should not be propagated.

Comment 21

[Boris Zbarsky [:bzbarsky]]

I filed bug 285188 on a related issue in frame loader.

Comment 22

[Bernd]

fix checked in