Closed Bug 428489 Opened 16 years ago Closed 16 years ago

Crash [@ nsHTMLEditor::GetPositionAndDimensions] when window gets removed during click on contenteditable absolute positioned element

Categories

(Core :: DOM: Editor, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9

People

(Reporter: martijn.martijn, Assigned: cpearce)

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(2 files, 1 obsolete file)

Attached file testcase
See testcase, which crashes current trunk build when clicking on the text in the iframe.

The iframe content consists of this:
<html><head>
<script>
window.addEventListener('DOMAttrModified', function(e) {window.frameElement.parentNode.removeChild(window.frameElement)}, true);
</script>
</head>
<body>
<div style="position: absolute;" contenteditable="true">Clicking on this should not crash Mozilla
</body>
</html>

http://crash-stats.mozilla.com/report/index/727788e3-07bd-11dd-8ce0-001b78bc73ea
0  	xul.dll  	nsHTMLEditor::GetPositionAndDimensions  	 mozilla/editor/libeditor/html/nsHTMLAnonymousUtils.cpp:441
1 	xul.dll 	nsDOMEventRTTearoff::Release 	mozilla/content/base/src/nsGenericElement.cpp:993
2 	xul.dll 	nsHTMLEditor::ShowGrabberOnElement 	mozilla/editor/libeditor/html/nsHTMLAbsPosition.cpp:356
3 	xul.dll 	nsHTMLEditor::IsModifiableNode 	mozilla/editor/libeditor/html/nsHTMLEditor.cpp:3978

If you want a regression range, let me know.
I think that when the frame is deleted, the CSS view is removed. Then the call to nsHTMLCSSUtils::GetDefaultViewCSS() at http://mxr.mozilla.org/seamonkey/source/editor/libeditor/html/nsHTMLAnonymousUtils.cpp#436 succeeds, but sets viewCSS to null, which we dereference at line 440. Simple fix is to return failure if GetDefaultViewCSS() set viewCSS to null rather than if it returned failure.
Assignee: nobody → chris
Status: NEW → ASSIGNED
Attachment #315423 - Flags: review?(roc)
Comment on attachment 315423 [details] [diff] [review]
Patch v1 - null check on nsHTMLCSSUtils::GetDefaultViewCSS() call

Requesting approval1.9, as this is a simple crasher fix with low risk.
Attachment #315423 - Flags: approval1.9?
(In reply to comment #2)
> (From update of attachment 315423 [details] [diff] [review])
> Requesting approval1.9, as this is a simple crasher fix with low risk.
> 

Can we add a crashtest for this?
Comment on attachment 315423 [details] [diff] [review]
Patch v1 - null check on nsHTMLCSSUtils::GetDefaultViewCSS() call

Re-request approval once we have addressed the crash test question above.
Attachment #315423 - Flags: approval1.9? → approval1.9-
Patch v1 with crashtest. r+/sr+ roc.
Attachment #315423 - Attachment is obsolete: true
Attachment #316139 - Flags: superreview+
Attachment #316139 - Flags: review+
Comment on attachment 316139 [details] [diff] [review]
V1 with crashtest

Low risk patch, fixes a crash, patch includes crashtest.
Attachment #316139 - Flags: approval1.9?
Comment on attachment 316139 [details] [diff] [review]
V1 with crashtest

a1.9=beltzner
Attachment #316139 - Flags: approval1.9? → approval1.9+
mozilla/editor/libeditor/html/nsHTMLAnonymousUtils.cpp 	1.21
mozilla/editor/libeditor/html/crashtests/428489-1.html 	1.1
mozilla/editor/libeditor/html/crashtests/crashtests.list 	1.5 
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
Crash Signature: [@ nsHTMLEditor::GetPositionAndDimensions]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: