Closed Bug 1324505 Opened 4 years ago Closed 4 years ago

SEGV near null in [@ GetParentNode]


(Core :: DOM: Editor, defect, P1)




Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- fixed


(Reporter: truber, Assigned: m_kato)


(Keywords: crash, testcase)

Crash Data


(4 files)

Attached file testcase.html
The attached testcase causes a SEGV near null in mozilla-central rev d4b3146a5567

==8702==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000028 (pc 0x7f190707ccb0 bp 0x7ffcfff07af0 sp 0x7ffcfff07960 T0)
    #0 0x7f190707ccaf in GetParentNode /home/worker/workspace/build/src/dom/base/nsINode.h:908:12
    #1 0x7f190707ccaf in mozilla::HTMLEditRules::PopListItem(nsIDOMNode*, bool*) /home/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:7849
    #2 0x7f190704d3cf in mozilla::HTMLEditRules::WillRemoveList(mozilla::dom::Selection*, bool, bool*, bool*) /home/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:3408:14
    #3 0x7f19070309c2 in mozilla::HTMLEditRules::WillDoAction(mozilla::dom::Selection*, mozilla::RulesInfo*, bool*, bool*) /home/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:636:14
    #4 0x7f19070c2519 in mozilla::HTMLEditor::RemoveList(nsAString_internal const&) /home/worker/workspace/build/src/editor/libeditor/HTMLEditor.cpp:2073:17
    #5 0x7f19071947ae in nsListCommand::ToggleState(nsIEditor*) /home/worker/workspace/build/src/editor/composer/nsComposerCommands.cpp:303:10
    #6 0x7f1907191189 in nsBaseStateUpdatingCommand::DoCommand(char const*, nsISupports*) /home/worker/workspace/build/src/editor/composer/nsComposerCommands.cpp:92:10
    #7 0x7f1908636066 in nsControllerCommandTable::DoCommand(char const*, nsISupports*) /home/worker/workspace/build/src/embedding/components/commandhandler/nsControllerCommandTable.cpp:147:10
    #8 0x7f190862ca2a in nsBaseCommandController::DoCommand(char const*) /home/worker/workspace/build/src/embedding/components/commandhandler/nsBaseCommandController.cpp:136:10
    #9 0x7f1908633376 in nsCommandManager::DoCommand(char const*, nsICommandParams*, mozIDOMWindowProxy*) /home/worker/workspace/build/src/embedding/components/commandhandler/nsCommandManager.cpp:214:10
    #10 0x7f19059cf408 in nsHTMLDocument::ExecCommand(nsAString_internal const&, bool, nsAString_internal const&, mozilla::dom::CallerType, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/html/nsHTMLDocument.cpp:3236:10
    #11 0x7f1904d977ce in mozilla::dom::HTMLDocumentBinding::execCommand(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/HTMLDocumentBinding.cpp:829:15
Priority: -- → P1
Assignee: nobody → m_kato
Crash Signature: [@ mozilla::HTMLEditRules::PopListItem ]
Comment on attachment 8826067 [details]
Bug 1324505 - Part 1. Check whether parent node is null.
Attachment #8826067 - Flags: review?(masayuki) → review+
Comment on attachment 8826069 [details]
Bug 1324505 - Part 3. Clean up HTMLEditRules::PopListItem.

::: editor/libeditor/HTMLEditRules.h:375
(Diff revision 1)
>                           int32_t& inOutOffset);
>    nsresult AddTerminatingBR(nsIDOMNode *aBlock);
>    EditorDOMPoint JoinNodesSmart(nsIContent& aNodeLeft,
>                                  nsIContent& aNodeRight);
>    Element* GetTopEnclosingMailCite(nsINode& aNode);
> -  nsresult PopListItem(nsIDOMNode* aListItem, bool* aOutOfList);
> +  nsresult PopListItem(nsIContent& aListItem, bool* aOutOfList = nullptr);

I wonder, when an argument is an instance of refcountable object, who should guarantee that it won't be released during the method? Callee? Caller?

I guess that it's callee because not all methods like this may cause releasing given instances.

So, please be careful when you change existing methods like this.

::: editor/libeditor/HTMLEditRules.cpp
(Diff revision 1)
> -  nsCOMPtr<Element> listItem = do_QueryInterface(aListItem);
> -  // check parms
> -

The original code here grabs aListItem's isntance, but after your patch is applied, it's not guaranteed. So, perhaps, you need to do something like this here:

nsCOMPtr<nsIContent> kungFuDeathGrip(&aListItem);

::: editor/libeditor/HTMLEditRules.cpp:7892
(Diff revision 1)
>    if (!bIsFirstListItem) {
>      parOffset++;
>    }
> -  rv = mHTMLEditor->MoveNode(listItem, curParPar, parOffset);
> +  nsresult rv = mHTMLEditor->MoveNode(&aListItem, curParPar, parOffset);

So, I hope aListItem won't be released at calling SplitNode() or something.
Attachment #8826069 - Flags: review?(masayuki) → review+
Pushed by
Part 1. Check whether parent node is null. r=masayuki
Part 2. Add crash test. r=masayuki
Part 3. Clean up HTMLEditRules::PopListItem. r=masayuki
I back out this due to mistake
19:34 (pulsebot) Check-in: - Makoto Kato - Backed out changeset 7be23bb65b93 (bug 1324505)
19:34 (pulsebot) Check-in: - Makoto Kato - Backed out changeset ea8744408e89 (bug 1324505)
19:34 (pulsebot) Check-in: - Makoto Kato - Backed out changeset f1f0f69bc78f (bug 1324505)
Pushed by
Part 1. Check whether parent node is null. r=masayuki
Part 2. Add crash test. r=masayuki
Part 3. Clean up HTMLEditRules::PopListItem. r=masayuki
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(m_kato)
This depends on design mode and It seems to be no crash expect to my test.  I don't think that we need uplift
Flags: needinfo?(m_kato)
You need to log in before you can comment on or make changes to this bug.