Bug 166710 - Crash inside Editor::styleForSelectionStart
Summary: Crash inside Editor::styleForSelectionStart
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: Safari 10
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-04 19:15 PST by Ryosuke Niwa
Modified: 2017-01-05 14:19 PST (History)
4 users (show)

See Also:

Attachments
Fixes the crash (1.88 KB, patch)
2017-01-04 19:42 PST, Ryosuke Niwa 		cdumez: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2017-01-04 19:15:59 PST 
0   com.apple.WebCore             	0x00007fffdbc6b2c4 WebCore::checkAcceptChild(WebCore::ContainerNode&, WebCore::Node&, WebCore::Node const*, WebCore::Document::AcceptChildOperation) + 36
1   com.apple.WebCore             	0x00007fffdbc6ceb7 WebCore::ContainerNode::appendChild(WebCore::Node&) + 39
2   com.apple.WebCore             	0x00007fffdbe26014 WebCore::Editor::styleForSelectionStart(WebCore::Frame*, WebCore::Node*&) + 628
3   com.apple.WebKit              	0x00007fffdd15bf98 WebKit::WebPage::editorState(WebKit::WebPage::IncludePostLayoutDataHint) const + 368
4   com.apple.WebKit              	0x00007fffdd15c357 WebKit::WebPage::updateEditorStateAfterLayoutIfEditabilityChanged() + 91
5   com.apple.WebCore             	0x00007fffdbeece3d WebCore::FrameSelection::updateAppearanceAfterLayout() + 45
6   com.apple.WebCore             	0x00007fffdb96aac1 WebCore::FrameView::performPostLayoutTasks() + 65
7   com.apple.WebCore             	0x00007fffdb9609b1 WebCore::FrameView::layout(bool) + 3969
8   com.apple.WebCore             	0x00007fffdb9c7f4b WebCore::Document::updateLayout() + 187
9   com.apple.WebCore             	0x00007fffdbdb1047 WebCore::Document::updateLayoutIgnorePendingStylesheets(WebCore::Document::RunPostLayoutTasks) + 295
10  com.apple.WebCore             	0x00007fffdb9f818d WebCore::Element::offsetTop() + 29
11  com.apple.WebCore             	0x00007fffdc21bff8 WebCore::jsElementOffsetTop(JSC::ExecState*, long long, JSC::PropertyName) + 72
12  com.apple.JavaScriptCore      	0x00007fffd75eef90 JSC::getByVal(JSC::ExecState*, JSC::JSValue, JSC::JSValue, JSC::ByValInfo*, JSC::ReturnAddressPtr) + 5760

<rdar://problem/29763079>
Comment 1 Ryosuke Niwa 2017-01-04 19:42:13 PST 
Created attachment 298060 [details]
Fixes the crash
Comment 2 Darin Adler 2017-01-05 00:50:15 PST 
Comment on attachment 298060 [details]
Fixes the crash

View in context: https://bugs.webkit.org/attachment.cgi?id=298060&action=review

> Source/WebCore/editing/cocoa/EditorCocoa.mm:79
> +    RefPtr<Node> positionNode = position.deprecatedNode();

Why not Node* (or auto or auto*) instead of RefPtr<Node>? Doesn’t seem necessary to churn the reference count here.
Comment 3 Ryosuke Niwa 2017-01-05 14:09:07 PST 
(In reply to comment #2)
> Comment on attachment 298060 [details]
> Fixes the crash
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298060&action=review
> 
> > Source/WebCore/editing/cocoa/EditorCocoa.mm:79
> > +    RefPtr<Node> positionNode = position.deprecatedNode();
> 
> Why not Node* (or auto or auto*) instead of RefPtr<Node>? Doesn’t seem
> necessary to churn the reference count here.

I'd use auto here. I think using a raw pointer in editing code is a good idea because if someone starts using that raw pointe after calling appendChild, that could turn into a security vulnerability. And the risk and the cost of having to think about all those implications everywhere in editing is worth avoiding one ref churn.
Comment 4 Ryosuke Niwa 2017-01-05 14:19:33 PST 
Committed r210376: <http://trac.webkit.org/changeset/210376>