Closed Bug 265181 Opened 21 years ago Closed 21 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: 21 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.

Attachment

General

Creator:
Created:
Updated:
Size: