crash due to bad table caption [@ CallQueryInterface]

VERIFIED FIXED

Status

()

Product:
Core
Component:
Layout: Tables
Type:
defect
Importance:
--
critical
Status:
VERIFIED FIXED
Reported:
15 years ago
Modified:
5 years ago

People

(Reporter: hellfire3k, Assigned: bernd_mozilla)

Tracking

({crash, fixed-aviary1.0, fixed1.7.5})

Version:
Trunk
Platform:
x86
Windows XP
Points:
---
Dependency tree / graph
Duplicates:
265375, 265376, 265583, 265585

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:nse], crash signature)

Attachments

(8 attachments)

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

Description

•
15 years ago 
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
(Reporter)

Comment 1

•
15 years ago 
That is the source code of the html document, and it's bogus.

Comment 2

•
15 years ago 
No sign of security exploit, just a crash.
Group: security
Whiteboard: [sg:nse]
(Assignee)

Comment 3

•
15 years ago 
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
(Assignee)

Comment 4

•
15 years ago 
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)

Updated

•
15 years ago
Assignee: general → nobody
QA Contact: general → core.layout.form-controls
 (Assignee)

Updated

•
15 years ago
Blocks: Zalewski

Comment 5

•
15 years ago 
Even though the crash is in a different place, the fix in bug 264956 might fix
this one as well.
 (Assignee)

Comment 6

•
15 years ago 
The patch does not fix this crash, it still crashes with winxp 2004101904 the
patch went in on 2004-10-18.

Comment 7

•
15 years ago 
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

Comment 9

•
15 years ago 
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

Comment 10

•
15 years ago 
Note that ContentInserted() handles captions differently from ContentAppended().
 One or the other is bogus.

Comment 11

•
15 years ago 
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.

Comment 12

•
15 years ago 
(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.

Comment 13

•
15 years ago 
That's because the caption didn't have an absolutely positioned input inside....
 Or worse yet an absolutely positioned animated image.

Comment 14

•
15 years ago 
(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

Comment 15

•
15 years ago 
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...
 (Assignee)

Comment 16

•
15 years ago 
Couldn't we avoid to delete the frames when we create another anonymous table
frame around the second caption?
 (Assignee)

Comment 17

•
15 years ago 
or do we need the full procedure like in
nsCSSFrameConstructor::WipeContainingBlock ? Boris?

Updated

•
15 years ago
Summary: crash @ [CallQueryInterface] → crash due to bad table caption [@ CallQueryInterface]

Comment 18

•
15 years ago 
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).
 (Assignee)

Comment 19

•
15 years ago 
comment 15 is bug 247148
 (Assignee)

Comment 21

•
15 years ago
Posted 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.
 (Assignee)

Comment 22

•
15 years ago
Posted 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.
 (Assignee)

Updated

•
15 years ago
Blocks: 265583

Comment 23

•
15 years ago 
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...
 (Assignee)

Updated

•
15 years ago
Blocks: 265585
 (Assignee)

Comment 24

•
15 years ago
Posted patch patch — — Splinter Review
(Assignee)

Updated

•
15 years ago
Assignee: nobody → bernd_mozilla
Status: NEW → ASSIGNED
 (Assignee)

Comment 25

•
15 years ago
Posted file testcase — 
this testcase shows that the expensive style resolution is not executed if a
row is appended.
 (Assignee)

Comment 26

•
15 years ago 
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)
 (Assignee)

Updated

•
15 years ago
Blocks: 265376

Comment 27

•
15 years ago 
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?
 (Assignee)

Comment 28

•
15 years ago
Posted file crash testcase — 
crash testcase that will still crash after the checkin for bug 265404

Comment 29

•
15 years ago 
(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.
 (Assignee)

Comment 30

•
15 years ago 
Daniel, and you do crash with the patch that is attached to this bug?

Comment 32

•
15 years ago 
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+
 (Assignee)

Comment 33

•
15 years ago
(Assignee)

Comment 34

•
15 years ago 
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?

Comment 35

•
15 years ago 
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+
 (Assignee)

Comment 36

•
15 years ago 
*** Bug 265375 has been marked as a duplicate of this bug. ***
 (Assignee)

Comment 37

•
15 years ago 
fix checked in into 1.7 and aviary
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Keywords: fixed-aviary1.0, fixed1.7.x
Resolution: --- → FIXED

Comment 38

•
15 years ago 
*** Bug 265583 has been marked as a duplicate of this bug. ***

Comment 39

•
15 years ago 
*** Bug 265585 has been marked as a duplicate of this bug. ***

Comment 40

•
15 years ago 
verified on windows xp firefox branch build 2004102607
Status: RESOLVED → VERIFIED
 (Assignee)

Updated

•
15 years ago
No longer blocks: 265376
 (Assignee)

Comment 41

•
15 years ago 
*** Bug 265376 has been marked as a duplicate of this bug. ***

Comment 42

•
15 years ago 
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?

Comment 43

•
15 years ago 
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.