Closed Bug 1558412 Opened 4 months ago Closed 4 months ago

Crash in [@ nsContentUtils::GetHTMLEditor]

Categories

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

x86
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- unaffected
firefox67.0.1 --- unaffected
firefox68 --- unaffected
firefox69 --- fixed

People

(Reporter: calixte, Assigned: masayuki)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files)

This bug is for crash report bp-623bdd31-097d-4f84-8a19-a08ec0190611.

Top 10 frames of crashing thread:

0 xul.dll nsContentUtils::GetHTMLEditor dom/base/nsContentUtils.cpp:6796
1 xul.dll mozilla::dom::Document::ExecCommand dom/base/Document.cpp:4324
2 xul.dll static bool mozilla::dom::Document_Binding::execCommand dom/bindings/DocumentBinding.cpp:3590
3 xul.dll mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> dom/bindings/BindingUtils.cpp:3171
4 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:540
5 xul.dll static bool Interpret js/src/vm/Interpreter.cpp:3087
6 xul.dll js::RunScript js/src/vm/Interpreter.cpp:425
7 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:568
8 xul.dll js::Call js/src/vm/Interpreter.cpp:611
9 xul.dll js::jit::InvokeFunction js/src/jit/VMFunctions.cpp:260

There are 16 crashes (from 2 installations) in nightly 69 with buildid 20190610214846. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1529884.

[1] https://hg.mozilla.org/mozilla-central/rev?node=258c4da663ca

Flags: needinfo?(masayuki)
No longer blocks: 1529884
Regressed by: 1529884
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)
Priority: -- → P1

Surprisingly, Document::ExecCommand() may be called when there is no
corresponding nsPresContext. Then, nsContentUtils::GetHTMLEditor()
crashes because it does not check whether the argument is nullptr or not.

This patch makes it check whether aPresContext is nullptr or not, and
makes Document::ExecCommand() not try to look for an editor when its
GetPresContext() returns nullptr. This means that we cannot send
proper nsIPrincipal object to the editor in this case. But I'm not sure
how it's important and whether editor can or cannot modify the DOM tree
actually.

Well, let's land the patch first for preventing the crash of Nightly testers.

Keywords: leave-open
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/9abe42390b6c
Make `nsContentUtils::GetHTMLEditor()` null-check of the given `nsPresContext*` r=smaug

While a document is invisible, execCommand() should do nothing in it.
This is not explicitly defined, but filed a spec issue:
https://github.com/w3c/editing/issues/193

Edge and Safari accept execCommand() and user inputs in invisible
<iframe>. That allows invisible <iframe> to steal user input. Therefore,
probably, also execCommand() should do nothing for implementing safer
behavior easier.

Duplicate of this bug: 1557607

The patch in comment 12 fixed the crashes. Let's move any follow-up work to a new bug for better tracking at this point.

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.