Closed
Bug 265181
Opened 19 years ago
Closed 19 years ago
crash due to bad table caption [@ CallQueryInterface]
Categories
(Core :: Layout: Tables, defect)
Tracking
()
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)
74 bytes,
text/html
|
Details | |
44 bytes,
text/html
|
Details | |
3.50 KB,
patch
|
Details | Diff | Splinter Review | |
3.73 KB,
patch
|
Details | Diff | Splinter Review | |
3.86 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
224 bytes,
text/html
|
Details | |
126 bytes,
text/html
|
Details | |
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
Reporter | ||
Comment 1•19 years ago
|
||
That is the source code of the html document, and it's bogus.
Comment 2•19 years ago
|
||
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
Comment 5•19 years ago
|
||
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
![]() |
||
Comment 8•19 years ago
|
||
![]() |
||
Comment 9•19 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•19 years ago
|
||
Note that ContentInserted() handles captions differently from ContentAppended(). One or the other is bogus.
![]() |
||
Comment 11•19 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•19 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•19 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•19 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•19 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•19 years ago
|
||
Couldn't we avoid to delete the frames when we create another anonymous table frame around the second caption?
Assignee | ||
Comment 17•19 years ago
|
||
or do we need the full procedure like in nsCSSFrameConstructor::WipeContainingBlock ? Boris?
Summary: crash @ [CallQueryInterface] → crash due to bad table caption [@ CallQueryInterface]
![]() |
||
Comment 18•19 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•19 years ago
|
||
comment 15 is bug 247148
Assignee | ||
Comment 20•19 years ago
|
||
maybe we should simply take the performance hit http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp&mark=8539-8542
Assignee | ||
Comment 21•19 years ago
|
||
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•19 years ago
|
||
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.
![]() |
||
Comment 23•19 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 | ||
Comment 24•19 years ago
|
||
Assignee | ||
Comment 25•19 years ago
|
||
this testcase shows that the expensive style resolution is not executed if a row is appended.
Assignee | ||
Comment 26•19 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)
Comment 27•19 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•19 years ago
|
||
crash testcase that will still crash after the checkin for bug 265404
Comment 29•19 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•19 years ago
|
||
Daniel, and you do crash with the patch that is attached to this bug?
Comment 31•19 years ago
|
||
WFM with patch (attachment 163108 [details] [diff] [review])
![]() |
||
Comment 32•19 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•19 years ago
|
||
Assignee | ||
Comment 34•19 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•19 years ago
|
||
WFM with Mozilla trunk CVS "after patch" build on Windows ME.
Updated•19 years ago
|
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•19 years ago
|
||
*** Bug 265375 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 37•19 years ago
|
||
fix checked in into 1.7 and aviary
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Keywords: fixed-aviary1.0,
fixed1.7.x
Resolution: --- → FIXED
![]() |
||
Comment 38•19 years ago
|
||
*** Bug 265583 has been marked as a duplicate of this bug. ***
![]() |
||
Comment 39•19 years ago
|
||
*** Bug 265585 has been marked as a duplicate of this bug. ***
Comment 40•19 years ago
|
||
verified on windows xp firefox branch build 2004102607
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 41•19 years ago
|
||
*** Bug 265376 has been marked as a duplicate of this bug. ***
Comment 42•19 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•19 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.
Updated•13 years ago
|
Crash Signature: [@ CallQueryInterface]
You need to log in
before you can comment on or make changes to this bug.
Description
•