Closed Bug 265181 Opened 18 years ago Closed 18 years ago

crash due to bad table caption [@ CallQueryInterface]

Categories

(Core :: Layout: Tables, defect)

Product:
Core
Component:
Layout: Tables
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: hellfire3k, Assigned: bernd_mozilla)

References

Details

(Keywords: crash, fixed-aviary1.0, fixed1.7.5, Whiteboard: [sg:nse])

Crash Data

Attachments

(8 files)

testcase ( crash on reload)
74 bytes, text/html
Details
Minimalish testcase
44 bytes, text/html
Details
idea
3.50 KB, patch
Details | Diff | Splinter Review
patch
3.73 KB, patch
Details | Diff | Splinter Review
patch
3.86 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
testcase
224 bytes, text/html
Details
crash testcase
126 bytes, text/html
Details
patch that got checked into trunk
3.83 KB, patch
dbaron
: approval-aviary+
dbaron
: approval1.7.5+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20041001 Firefox/0.10.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20041001 Firefox/0.10.1

<ãPARAM>
<TABLE>
<HTML about:>
<CAPTION><CAPTION >
<INPUT  <EMBED>

Reproducible: Always
Steps to Reproduce:
1. Open html

Actual Results:  
Crash

Expected Results:  
Outputted a error dialog and continued
That is the source code of the html document, and it's bogus.
No sign of security exploit, just a crash.
Group: security
Whiteboard: [sg:nse]
load the testcase
once it is loaded hit reload

CallQueryInterface(nsIFrame * 0x0366d448, nsIFormControlFrame * * 0x0012e404)
line 225 + 16 bytes
nsGenericHTMLElement::GetFormControlFrameFor(nsIContent * 0x0366e5b0,
nsIDocument * 0x03615888, int 0x00000000) line 2248 + 12 bytes
nsHTMLInputElement::GetValue(nsHTMLInputElement * const 0x0366e5f0, nsAString &
{...}) line 598 + 48 bytes
nsHTMLInputElement::SaveState(nsHTMLInputElement * const 0x0366e5cc) line 2411
nsGenericHTMLFormElement::SetDocument(nsIDocument * 0x00000000, int 0x00000001,
int 0x00000001) line 3356
nsHTMLInputElement::SetDocument(nsIDocument * 0x00000000, int 0x00000001, int
0x00000001) line 1684
nsGenericElement::SetDocumentInChildrenOf(nsIContent * 0x0366e470, nsIDocument
* 0x00000000, int 0x00000001) line 1718
nsGenericElement::SetDocument(nsIDocument * 0x00000000, int 0x00000001, int
0x00000001) line 1782 + 14 bytes
nsGenericHTMLElement::SetDocument(nsIDocument * 0x00000000, int 0x00000001, int
0x00000001) line 1313
nsGenericElement::SetDocumentInChildrenOf(nsIContent * 0x03672fa8, nsIDocument
* 0x00000000, int 0x00000001) line 1718
nsGenericElement::SetDocument(nsIDocument * 0x00000000, int 0x00000001, int
0x00000001) line 1782 + 14 bytes
nsGenericHTMLElement::SetDocument(nsIDocument * 0x00000000, int 0x00000001, int
0x00000001) line 1313
nsGenericElement::SetDocumentInChildrenOf(nsIContent * 0x0365c330, nsIDocument
* 0x00000000, int 0x00000001) line 1718
nsGenericElement::SetDocument(nsIDocument * 0x00000000, int 0x00000001, int
0x00000001) line 1782 + 14 bytes
nsGenericHTMLElement::SetDocument(nsIDocument * 0x00000000, int 0x00000001, int
0x00000001) line 1313
nsHTMLBodyElement::SetDocument(nsIDocument * 0x00000000, int 0x00000001, int
0x00000001) line 444
nsGenericElement::SetDocumentInChildrenOf(nsIContent * 0x03608108, nsIDocument
* 0x00000000, int 0x00000001) line 1718
nsGenericElement::SetDocument(nsIDocument * 0x00000000, int 0x00000001, int
0x00000001) line 1782 + 14 bytes
nsGenericHTMLElement::SetDocument(nsIDocument * 0x00000000, int 0x00000001, int
0x00000001) line 1313
nsDocument::SetScriptGlobalObject(nsIScriptGlobalObject * 0x00000000) line 1792

DocumentViewerImpl::Close(DocumentViewerImpl * const 0x03606f88) line 1126
nsDocShell::SetupNewViewer(nsDocShell * const 0x03480cc0, nsIContentViewer *
0x0350d788) line 4800
nsDocShell::Embed(nsDocShell * const 0x03480ce4, nsIContentViewer * 0x0350d788,
const char * 0x026a57e8 `string', nsISupports * 0x00000000) line 4132 + 22
bytes
nsDocShell::CreateContentViewer(nsDocShell * const 0x03480cc0, const char *
0x02eae0a8, nsIRequest * 0x035d02c8, nsIStreamListener * * 0x02ea16b0) line
4550 + 26 bytes
nsDSURIContentListener::DoContent(nsDSURIContentListener * const 0x03480e48,
const char * 0x02eae0a8, int 0x00000000, nsIRequest * 0x035d02c8,
nsIStreamListener * * 0x02ea16b0, int * 0x0012f058) line 126 + 30 bytes
nsDocumentOpenInfo::TryContentListener(nsIURIContentListener * 0x03480e48,
nsIChannel * 0x035d02c8) line 719 + 124 bytes
nsDocumentOpenInfo::DispatchContent(nsIRequest * 0x035d02c8, nsISupports *
0x00000000) line 450 + 63 bytes
nsDocumentOpenInfo::OnStartRequest(nsDocumentOpenInfo * const 0x02ea16a0,
nsIRequest * 0x035d02c8, nsISupports * 0x00000000) line 321 + 14 bytes
nsFileChannel::OnStartRequest(nsFileChannel * const 0x035d02d0, nsIRequest *
0x02e4bb50, nsISupports * 0x00000000) line 530
nsInputStreamPump::OnStateStart() line 379 + 92 bytes
nsInputStreamPump::OnInputStreamReady(nsInputStreamPump * const 0x02e4bb54,
nsIAsyncInputStream * 0x035ee650) line 335 + 11 bytes
nsInputStreamReadyEvent::EventHandler(PLEvent * 0x0330b2dc) line 119
PL_HandleEvent(PLEvent * 0x0330b2dc) line 692 + 9 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00deae30) line 627 + 8 bytes
nsEventQueueImpl::ProcessPendingEvents(nsEventQueueImpl * const 0x00de4a18)
line 391 + 11 bytes
nsWindow::DispatchPendingEvents() line 3736
nsWindow::ProcessMessage(unsigned int 0x00000202, unsigned int 0x00000000, long
0x002b00b2, long * 0x0012fbb8) line 4060
nsWindow::WindowProc(HWND__ * 0x0002036e, unsigned int 0x00000202, unsigned int
0x00000000, long 0x002b00b2) line 1355 + 24 bytes
USER32! 77d18709()
USER32! 77d187eb()
USER32! 77d189a5()
USER32! 77d189e8()
nsAppShell::Run(nsAppShell * const 0x00ea5b80) line 135
nsAppShellService::Run(nsAppShellService * const 0x00ea58f0) line 489
main1(int 0x00000003, char * * 0x002a2d00, nsISupports * 0x00e35f40) line 1321
+ 31 bytes
main(int 0x00000003, char * * 0x002a2d00) line 1799 + 34 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 7c816
we call CallQueryInterface on previously deleted frame
Status: UNCONFIRMED → NEW
Component: Browser-General → Layout: Form Controls
Ever confirmed: true
Keywords: crash
Summary: This webpage crashes mozilla in windows on both firefox and mozilla → crash @ [CallQueryInterface]
Assignee: general → nobody
QA Contact: general → core.layout.form-controls
Blocks: Zalewski
Even though the crash is in a different place, the fix in bug 264956 might fix
this one as well.
The patch does not fix this crash, it still crashes with winxp 2004101904 the
patch went in on 2004-10-18.
Tried to reduce it abit, manage to crash it with this line:

<ã><table><html a><caption><caption><input>

dom looks something like this, if I replace ã with a null character it won't
crash until I reload or use dom inspector to inspect the <input> element
html
  head
  body
    <ã> <--text node
    table
      caption
      caption
        input

So the problem here is that the frame construction code for captions in
ContentAppended() Destroy()s the frame list if it decides it doesn't like
things.  The problem is that it never removes those frames from the primary
frame map, and the TextControlFrame is in the primary frame map by that point. 
Which is correct, since ConstructFrame() _should_ add the frame to the primary
frame map.
Component: Layout: Form Controls → Layout: Tables
QA Contact: core.layout.form-controls → core.layout.tables
Note that ContentInserted() handles captions differently from ContentAppended().
 One or the other is bogus.
Also note that calling CleanupFrameReferences() on the caption before calling
Destroy() would help a bit here, but we'd also need to drop out of flow kids of
the caption we're dropping from the abs-pos/fixed-pos/float lists on the frame
construction state.
(In reply to comment #10)
> Note that ContentInserted() handles captions differently from ContentAppended().
>  One or the other is bogus.
looks like a leak in ContentInserted() since it does not check the return value
of the AppendFrames() call (which can be NS_ERROR_UNEXPECTED if there is already
a caption).

(In reply to comment #11)
> Also note that calling CleanupFrameReferences() on the caption before calling
> Destroy() would help a bit here, but we'd also need to drop out of flow kids of
> the caption we're dropping from the abs-pos/fixed-pos/float lists on the frame
> construction state.
I just tested calling DoCleanupFrameReferences() on the caption before calling
Destroy(), no more crash.


That's because the caption didn't have an absolutely positioned input inside....
 Or worse yet an absolutely positioned animated image.
(In reply to comment #13)
> That's because the caption didn't have an absolutely positioned input inside....
>  Or worse yet an absolutely positioned animated image.
would calling DeletingFrameSubtree do the trick?
http://lxr.mozilla.org/mozilla/source/layout/html/style/src/nsCSSFrameConstructor.cpp#9416
No, since the deleted frames would still hang out in the temporary framelists on
the frame constructor state.

It may be possible to flag all the frames in the lists, then call
DeletingFrameSubtree, then remove the ones that have the flag set or something
along those lines...
Blocks: 265375
Couldn't we avoid to delete the frames when we create another anonymous table
frame around the second caption?
or do we need the full procedure like in
nsCSSFrameConstructor::WipeContainingBlock ? Boris?
Summary: crash @ [CallQueryInterface] → crash due to bad table caption [@ CallQueryInterface]
I we created an anonymous table child of the first caption, then we could
probably avoid deleting the frames, yes...  assuming I understand what this code
is trying to do (which is that it can't deal with two captions in one table, so
it just tries to ditch one).
comment 15 is bug 247148
Attached patch idea — — Splinter Review 
I dont know how to get the display type for the content, but with this approach
we could stop the frame creation that we cannot handle.
Attached patch patch — — Splinter Review 
Boris, could you look at the patch, the idea here is a change from a scenario
where we remove a unwanted frame to a scenario where we don't create that frame
at all.
Blocks: 265583
That seems reasonable (though the abortCaption boolean seems unused now, no?)

I sort of wonder whether we could push this check down into
ConstructFrameByDisplay() (where we already have the style context), though... 
I guess we'd need to pass in the "already has a caption" state then, eh?

What I'm worried about is that ContentAppended() can get called quite a bit on a
table in a page with a long table.  And we'd be adding extra style resolution to
that...
Blocks: 265585
Attached patch patch — — Splinter Review
Assignee: nobody → bernd_mozilla
Status: NEW → ASSIGNED
Attached file testcase — 
this testcase shows that the expensive style resolution is not executed if a
row is appended.
Comment on attachment 163108 [details] [diff] [review]
patch

Boris, I think I would like to go with this. The case that you mentioned will
not  happen as when we append rows to a long table they will have a rowgroup
frame as a parentFrame so this code will only get called when we add a rowgroup
colgroup or a caption frame. Unless somebody uses a table where every row is
encapsulated in a rowgroup this will not be an issue.
I like the patch more than pushing a hasCaption argument through the whole
frame construction.
Attachment #163108 - Flags: superreview?(bzbarsky)
Attachment #163108 - Flags: review?(bzbarsky)
Blocks: 265376
This as well as the dependents bug 265585 and bug 265583 seem to be fixed today.
I do not get a crash any more with the build I just created on OS/2 from the
trunk. (I presume that the checkin from the hidden bug 265404 is responsible.)
Can anyone confirm?
Attached file crash testcase — 
crash testcase that will still crash after the checkin for bug 265404
(In reply to comment #28)
> Created an attachment (id=163159)
> crash testcase

First I got an assertion in "nsHTMLReflowState::InitAbsoluteConstraints" with
the comment "no placeholder frame". Then it crashes in
"GetNearestContainingBlock" (in nshtmlreflowstate.cpp) because the placeholder
is used there without any additional check.
Daniel, and you do crash with the patch that is attached to this bug?
Comment on attachment 163108 [details] [diff] [review]
patch

>Index: nsCSSFrameConstructor.cpp
>+      nsIFrame* existingCaption =
>+      outerTable->GetFirstChild(nsLayoutAtoms::captionList);
>+      if (existingCaption) {

Just make that:

       if (outerTable->GetFirstChild(nsLayoutAtoms::captionList)) {

>+    // and all it childs

s/childs/descendants/

>+          continue; //dont create a table caption frame and its childs

Same.

>         if (nsLayoutAtoms::tableCaptionFrame == tempItems.childList->GetType()) {
>+            // check if a caption already exists in captionItems, and if so, abort the caption
>           if (captionItems.childList) {
>+              hasCaption = PR_TRUE;
>           }
>+          captionItems.AddChild(tempItems.childList);        

Why do you need that |captionItems.childList| check?  Remove it, please.  As
written, this code is wrong.  You may want to assert that
|captionItems.childList == nsnull|, since that should be the case here if our
display check above worked.  Also, the comment should just say "remember that
we have a caption now" or something.

r+sr=bzbarsky with those changes.
Attachment #163108 - Flags: superreview?(bzbarsky)
Attachment #163108 - Flags: superreview+
Attachment #163108 - Flags: review?(bzbarsky)
Attachment #163108 - Flags: review+
Comment on attachment 163322 [details] [diff] [review]
patch that got checked into trunk

the crash fix should also go on branch it did not show performance regression
on btek.
Attachment #163322 - Flags: approval1.7.x?
Attachment #163322 - Flags: approval-aviary?
WFM with Mozilla trunk CVS "after patch" build on Windows ME.
Attachment #163322 - Flags: approval1.7.x?
Attachment #163322 - Flags: approval1.7.x+
Attachment #163322 - Flags: approval-aviary?
Attachment #163322 - Flags: approval-aviary+
*** Bug 265375 has been marked as a duplicate of this bug. ***
fix checked in into 1.7 and aviary
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Keywords: fixed-aviary1.0, fixed1.7.x
Resolution: --- → FIXED
*** Bug 265583 has been marked as a duplicate of this bug. ***
*** Bug 265585 has been marked as a duplicate of this bug. ***
verified on windows xp firefox branch build 2004102607
Status: RESOLVED → VERIFIED
No longer blocks: 265376
*** Bug 265376 has been marked as a duplicate of this bug. ***
Not sure about this, but could it have caused a regression?  I'm seeing extra
whitespace on these two pages:
http://mycroft.mozdev.org/download.html
http://daisydunkeld.com/blog/index.php?id=P319

Since it seems to be a table-related issue, and this is the only checkin in the
past few days which is related to that, I'm wondering if this caused it.  Any
thoughts?
Nevermind.  I doubt this is what caused the problem.  Some people on the
MozillaZine forums are reporting the same issue with builds that came out before
this fix was checked in.
Crash Signature: [@ CallQueryInterface]
You need to log in before you can comment on or make changes to this bug.