Closed Bug 1324505 Opened 7 years ago Closed 7 years ago

SEGV near null in [@ GetParentNode]

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- fixed

People

(Reporter: truber, Assigned: m_kato)

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(4 files)

testcase.html
750 bytes, text/html
Details
Bug 1324505 - Part 1. Check whether parent node is null.
59 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1324505 - Part 2. Add crash test.
59 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1324505 - Part 3. Clean up HTMLEditRules::PopListItem.
59 bytes, text/x-review-board-request
masayuki
: review+
Details
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.

https://reviewboard.mozilla.org/r/104114/#review104848
Attachment #8826067 - Flags: review?(masayuki) → review+
Comment on attachment 8826068 [details]
Bug 1324505 - Part 2. Add crash test.

https://reviewboard.mozilla.org/r/104116/#review104852
Attachment #8826068 - Flags: review?(masayuki) → review+
Comment on attachment 8826069 [details]
Bug 1324505 - Part 3. Clean up HTMLEditRules::PopListItem.

https://reviewboard.mozilla.org/r/104118/#review104854

::: 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
> -  NS_ENSURE_TRUE(listItem && aOutOfList, NS_ERROR_NULL_POINTER);
> -

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++;
>    }
>  
>    NS_ENSURE_STATE(mHTMLEditor);
> -  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 m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1f0f69bc78f
Part 1. Check whether parent node is null. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea8744408e89
Part 2. Add crash test. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/7be23bb65b93
Part 3. Clean up HTMLEditRules::PopListItem. r=masayuki
I back out this due to mistake
19:34 (pulsebot) Check-in: https://hg.mozilla.org/integration/mozilla-inbound/rev/72c812f1512c - Makoto Kato - Backed out changeset 7be23bb65b93 (bug 1324505)
19:34 (pulsebot) Check-in: https://hg.mozilla.org/integration/mozilla-inbound/rev/948912e1d62d - Makoto Kato - Backed out changeset ea8744408e89 (bug 1324505)
19:34 (pulsebot) Check-in: https://hg.mozilla.org/integration/mozilla-inbound/rev/ee2a1ad506f0 - Makoto Kato - Backed out changeset f1f0f69bc78f (bug 1324505)
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f96c5f0c2a0
Part 1. Check whether parent node is null. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f22ce6afa75
Part 2. Add crash test. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/86dde0e30819
Part 3. Clean up HTMLEditRules::PopListItem. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/9f96c5f0c2a0
https://hg.mozilla.org/mozilla-central/rev/4f22ce6afa75
https://hg.mozilla.org/mozilla-central/rev/86dde0e30819
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Please request Aurora approval on this when you get a chance.
status-firefox50: --- → wontfix
status-firefox51: --- → wontfix
status-firefox52: --- → affected
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.