Null-deref crash [@ GetThread]

RESOLVED FIXED in Firefox 52

Status

()

P1
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: truber, Assigned: m_kato)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
mozilla53
crash, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox50 wontfix, firefox51 wontfix, firefox52 fixed, firefox53 fixed)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Created attachment 8810860 [details]
testcase.html

The attached testcase causes a null pointer dereference in m-c version f8ba9c9b401f.

==2109==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000030 (pc 0x7fa31f2938a1 bp 0x7ffef3292d90 sp 0x7ffef3292d70 T0)
    #0 0x7fa31f2938a0 in GetThread src/obj-firefox/dist/include/nsISupportsImpl.h:58:36
    #1 0x7fa31f2938a0 in nsRange::AddRef() src/dom/base/nsRange.cpp:324
    #2 0x7fa322b5f18d in AddRef src/obj-firefox/dist/include/mozilla/RefPtr.h:37:5
    #3 0x7fa322b5f18d in AddRef src/obj-firefox/dist/include/mozilla/RefPtr.h:396
    #4 0x7fa322b5f18d in assign_with_AddRef src/obj-firefox/dist/include/mozilla/RefPtr.h:54
    #5 0x7fa322b5f18d in operator= src/obj-firefox/dist/include/mozilla/RefPtr.h:191
    #6 0x7fa322b5f18d in init<nsRange *> src/obj-firefox/dist/include/mozilla/OwningNonNull.h:147
    #7 0x7fa322b5f18d in OwningNonNull src/obj-firefox/dist/include/mozilla/OwningNonNull.h:25
    #8 0x7fa322b5f18d in mozilla::HTMLEditor::RemoveInlinePropertyImpl(nsIAtom*, nsAString_internal const*) src/editor/libeditor/HTMLStyleEditor.cpp:1235
    #9 0x7fa322b5e852 in mozilla::HTMLEditor::RemoveAllInlineProperties() src/editor/libeditor/HTMLStyleEditor.cpp:1179:17
    #10 0x7fa322beb593 in nsRemoveStylesCommand::DoCommand(char const*, nsISupports*) src/editor/composer/nsComposerCommands.cpp:1166:10
    #11 0x7fa32405c0b6 in nsControllerCommandTable::DoCommand(char const*, nsISupports*) src/embedding/components/commandhandler/nsControllerCommandTable.cpp:147:10
    #12 0x7fa324052aea in nsBaseCommandController::DoCommand(char const*) src/embedding/components/commandhandler/nsBaseCommandController.cpp:136:10
    #13 0x7fa3240593c6 in nsCommandManager::DoCommand(char const*, nsICommandParams*, mozIDOMWindowProxy*) src/embedding/components/commandhandler/nsCommandManager.cpp:214:10
    #14 0x7fa321416238 in nsHTMLDocument::ExecCommand(nsAString_internal const&, bool, nsAString_internal const&, mozilla::dom::CallerType, mozilla::ErrorResult&) src/dom/html/nsHTMLDocument.cpp:3228:10
(Assignee)

Updated

2 years ago
Priority: -- → P2
(Assignee)

Updated

2 years ago
Priority: P2 → P1
(Assignee)

Updated

2 years ago
Crash Signature: [@ mozilla::HTMLEditor::PromoteInlineRange ]
(Assignee)

Updated

2 years ago
Crash Signature: [@ mozilla::HTMLEditor::PromoteInlineRange ] → [@ mozilla::HTMLEditor::PromoteInlineRange ] [@ nsRange::AddRef ]
(Assignee)

Updated

2 years ago
Assignee: nobody → m_kato
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8818805 [details]
Bug 1317704 - Part 1. Hold current ranges into RemoveInlineProperty.

https://reviewboard.mozilla.org/r/98752/#review99068

::: editor/libeditor/HTMLStyleEditor.cpp:1234
(Diff revision 1)
> +    // Since ranges might be modified by SplitStyleAboveRange, we need hold
> +    // current ranges
> +    AutoTArray<OwningNonNull<nsRange>, 8> arrayOfRanges;
>      for (uint32_t rangeIdx = 0; rangeIdx < rangeCount; ++rangeIdx) {
> -      OwningNonNull<nsRange> range = *selection->GetRangeAt(rangeIdx);
> +      arrayOfRanges.AppendElement(*selection->GetRangeAt(rangeIdx));
> +    }
> +    for (auto& range : arrayOfRanges) {

I don't understand your change because according to your comment, we need to hold selection ranges before performing the comment and run the command for every range.

However, as far as I know, there is only one range when the selection is in an editor. So, the old code looks safe to me. And the automated test also creates only one range. So, what am I misunderstanding?
Er, my memory was wrong. We can do multiple selection in contenteditable editor either with mouse operation or JS.
https://jsfiddle.net/d_toybox/9Lrs62po/

So, your patch actually fixes a bug, but I'm still not sure why it fixes the crash written as automated test.
(Oh, and the testcase doesn't work as expected on Chrome nor Edge. Perhaps, we might need to stop multiple selection in editor...)
(Assignee)

Comment 6

2 years ago
SplitNodeDeep will modify nodes... and document.designMode = 'on'; creates additional ranges... humm....
So, you mean, when you run the automated test, the rangeCount at the for loop is 2 or more?
(Assignee)

Comment 8

2 years ago
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #7)
> So, you mean, when you run the automated test, the rangeCount at the for
> loop is 2 or more?

When calling "document.execCommand('removeformat', false, null);" on fuzzying code, range count is 3.  But by SliptNodeDeep, range count becomes 2.  I might have to use mRangeUpdater if possible.
Wow, I'm surprised but I see. So, the rangeCount becomes 3 (or 2) with the testcase, your patch does make sense. Although, looks like we have another bug.

Comment 10

2 years ago
mozreview-review
Comment on attachment 8818805 [details]
Bug 1317704 - Part 1. Hold current ranges into RemoveInlineProperty.

https://reviewboard.mozilla.org/r/98752/#review99262
Attachment #8818805 - Flags: review?(masayuki) → review+

Comment 11

2 years ago
mozreview-review
Comment on attachment 8818806 [details]
Bug 1317704 - Part 2. Add test.

https://reviewboard.mozilla.org/r/98754/#review99264

::: editor/libeditor/crashtests/1317704.html:11
(Diff revision 1)
> +addEventListener('DOMContentLoaded', function(){
> + document.documentElement.className = 'lizard';
> + setTimeout(function(){
> +  document.execCommand('selectAll', false, null);
> +  document.designMode = 'on';
> +  document.execCommand('removeformat', false, null);

Although, if this were a mochitest, we could check rangeCount with todo_is().
Attachment #8818806 - Flags: review?(masayuki) → review+
(Assignee)

Comment 12

2 years ago
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #11)
> Comment on attachment 8818806 [details]
> Bug 1317704 - Part 2. Add test.
> 
> https://reviewboard.mozilla.org/r/98754/#review99264
> 
> ::: editor/libeditor/crashtests/1317704.html:11
> (Diff revision 1)
> > +addEventListener('DOMContentLoaded', function(){
> > + document.documentElement.className = 'lizard';
> > + setTimeout(function(){
> > +  document.execCommand('selectAll', false, null);
> > +  document.designMode = 'on';
> > +  document.execCommand('removeformat', false, null);
> 
> Although, if this were a mochitest, we could check rangeCount with todo_is().

This is crash test.
Yes, therefore, I said "this were".

Comment 14

2 years ago
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/0951f650ba45
Part 1. Hold current ranges into RemoveInlineProperty. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/89f2b6b6c720
Part 2. Add test. r=masayuki

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0951f650ba45
https://hg.mozilla.org/mozilla-central/rev/89f2b6b6c720
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
>    AutoTArray<OwningNonNull<nsRange>, 8> arrayOfRanges;
>    for (uint32_t rangeIdx = 0; rangeIdx < rangeCount; ++rangeIdx) {
>      arrayOfRanges.AppendElement(*selection->GetRangeAt(rangeIdx));
>    }

Perhaps we should make that into a RAII class so it's easy to use
it in other places too?

And while we're at it, why not implement a ApplyToSelectionRanges method
that takes a lambda (the for-loop body that follows the above code) that
first creates that RAII stack object and then applies the lambda to each
range?
Please nominate this for Aurora approval.
status-firefox50: --- → wontfix
status-firefox51: --- → wontfix
status-firefox52: --- → affected
status-firefox-esr45: --- → unaffected
Flags: needinfo?(m_kato)
Flags: in-testsuite+
(Assignee)

Comment 18

2 years ago
Although this is crash bug, there is no crash data except to crash test.  So uplift isn't required.
Flags: needinfo?(m_kato)
status-firefox52: affected → wontfix
crash-stats does show a number of reports with the [@ mozilla::HTMLEditor::PromoteInlineRange ] signature, but I guess it's a different issue?
Flags: needinfo?(m_kato)
(Assignee)

Comment 20

2 years ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #19)
> crash-stats does show a number of reports with the [@
> mozilla::HTMLEditor::PromoteInlineRange ] signature, but I guess it's a
> different issue?

Ah, there is a few crash data except to my crash test.  OK, I will request uplift for beta
Flags: needinfo?(m_kato)
status-firefox52: wontfix → affected
(Assignee)

Comment 21

2 years ago
Comment on attachment 8818805 [details]
Bug 1317704 - Part 1. Hold current ranges into RemoveInlineProperty.

Approval Request Comment
[Feature/Bug causing the regression]:
No

[User impact if declined]:
when using designMode (document.designMode = 'on'), calling document.execCommand('removeformat') causes crash.

[Is this code covered by automated tests?]:
Test case is included in this bug.

[Has the fix been verified in Nightly?]:
Yes, landed in 53.

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
No

[Is the change risky?]:
Low.

[Why is the change risky/not risky?]:
By this fix, after we copy elements that are selected, then we access it.

[String changes made/needed]:
No
Attachment #8818805 - Flags: approval-mozilla-beta?
(Assignee)

Updated

2 years ago
Attachment #8818806 - Flags: approval-mozilla-beta?
Comment on attachment 8818805 [details]
Bug 1317704 - Part 1. Hold current ranges into RemoveInlineProperty.

crash fix, beta52+
Attachment #8818805 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8818806 [details]
Bug 1317704 - Part 2. Add test.

add a test for part 1, beta52+
Attachment #8818806 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.