Closed Bug 1342417 Opened 3 years ago Closed 2 years ago

Crash in mozilla::EditorBase::InsertNode

Categories

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

49 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 55+ fixed
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: philipp, Assigned: m_kato)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-e9e2f301-050a-441b-9ce3-14c5f2170224.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::EditorBase::InsertNode(nsIContent&, nsINode&, int) 	editor/libeditor/EditorBase.cpp:1403
1 	xul.dll 	mozilla::EditorBase::MoveNode(nsIContent*, nsINode*, int) 	editor/libeditor/EditorBase.cpp:1736
2 	xul.dll 	mozilla::HTMLEditor::ClearStyle(nsCOMPtr<nsINode>*, int*, nsIAtom*, nsAString_internal const*) 	editor/libeditor/HTMLStyleEditor.cpp:655
3 	xul.dll 	mozilla::HTMLEditor::DoInsertHTMLWithContext(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, nsIDOMDocument*, nsIDOMNode*, int, bool, bool, bool) 	editor/libeditor/HTMLEditorDataTransfer.cpp:348
4 	xul.dll 	mozilla::HTMLEditor::InsertFromTransferable(nsITransferable*, nsIDOMDocument*, nsAString_internal const&, nsAString_internal const&, bool, nsIDOMNode*, int, bool) 	editor/libeditor/HTMLEditorDataTransfer.cpp:1231
5 	xul.dll 	mozilla::HTMLEditor::Paste(int) 	editor/libeditor/HTMLEditorDataTransfer.cpp:1515
6 	xul.dll 	mozilla::PasteCommand::DoCommand(char const*, nsISupports*) 	editor/libeditor/EditorCommands.cpp:497
7 	xul.dll 	nsBaseCommandController::DoCommand(char const*) 	embedding/components/commandhandler/nsBaseCommandController.cpp:136

this cross-platform crash signature seems to be regressing in volume since firefox 49 and later - user comments generally refer to pasting something into the browser, which triggered the crash...
Priority: -- → P3
Mass wontfix for bugs affecting firefox 52.
Too late for a fix for 53 since we are a week away from the release. This is fairly low volume and has been triaged already by m_kato so I'm dropping this from regression triage.
HTMLEditor::ClearStyle modifies nodes by SplitStyleAbovePoint.  2nd SplitStyleAbovePoint returns NS_OK, but leftNode and/or rightNode doesn't return any value since there is no splitable node.  So parent node sets nullptr, then this crash will occur.
Assignee: nobody → m_kato
Crash Signature: [@ mozilla::EditorBase::InsertNode] [@ nsEditor::InsertNode] → [@ mozilla::EditorBase::InsertNode] [@ nsEditor::InsertNode] [@ mozilla::HTMLEditor::RemoveStyleInside]
Comment on attachment 8871627 [details]
Bug 1342417 - ClearStyle should check that SplitStyleAbovePoint returns node.

https://reviewboard.mozilla.org/r/143128/#review147268
Attachment #8871627 - Flags: review?(masayuki) → review+
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/51dc277d8fe8
ClearStyle should check that SplitStyleAbovePoint returns node. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/51dc277d8fe8
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Is this something we should let ride the 55 train at this point?
Flags: needinfo?(m_kato)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)
> Is this something we should let ride the 55 train at this point?

This is a crash bug, and this occurs when user paste HTML from clipboard.  Most situation is that user is writing a mail on gmail or writing a data into online form.  The crash in this situation causes more negative impact for Firefox.

If 54, this is too late to land this.
Flags: needinfo?(m_kato)
If you feel strongly that this should be uplifted to Beta and ESR52, please go ahead and nominate it for approval ASAP. I've already confirmed that it grafts cleanly to both branches.
Flags: needinfo?(m_kato)
Comment on attachment 8871627 [details]
Bug 1342417 - ClearStyle should check that SplitStyleAbovePoint returns node.

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

[User impact if declined]:
This occurs when user paste HTML from clipboard.  Most situation is that user is writing a mail on gmail or writing a data into online form.  The crash in this situation causes more negative impact for Firefox.

[Is this code covered by automated tests?]:
No, I cannot create automated test.  But crash reason is clear.

[Has the fix been verified in Nightly?]:
I cannot reproduce this.

[Needs manual test from QE? If yes, steps to reproduce]:
No
 
[List of other uplifts needed for the feature/fix]:
Nothing.

[Is the change risky?]:
Low.

[Why is the change risky/not risky?]:
This adds nullptr check only.

[String changes made/needed]:
No.
Flags: needinfo?(m_kato)
Attachment #8871627 - Flags: approval-mozilla-beta?
Comment on attachment 8871627 [details]
Bug 1342417 - ClearStyle should check that SplitStyleAbovePoint returns node.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
This crash occurs when user paste HTML from clipboard.  ESR users will use a lot of HTML form for enterprise document,  And they also use online mail service such as Google Apps and Office 365.  It means that crash rather occurs on ESR users than home users.

User impact if declined:
This occurs when user paste HTML from clipboard.  Most situation is that user is writing a mail on gmail or writing a data into online form.

Fix Landed on Version:
55

Risk to taking this patch (and alternatives if risky): 
Low.  This adds nullptr check only.

String or UUID changes made by this patch: 
No.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8871627 - Flags: approval-mozilla-esr52?
I'd like this to bake on Nightly a bit more before we decide whether to take it or not. As such from the crash volume, I do not believe this is a must fix for 54 so late in the beta cycle.
Comment on attachment 8871627 [details]
Bug 1342417 - ClearStyle should check that SplitStyleAbovePoint returns node.

Given the volume of crashes is low in 54 and it's late in Beta54. Let's let it ride the train. Beta54-.
Attachment #8871627 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
I think we should let this hangout for a bit, and possibly uplift for ESR52.3. This is a wontfix for ESR52.2 due to low crash volume in the first place and not enough stabilization yet on Nightly55.
Comment on attachment 8871627 [details]
Bug 1342417 - ClearStyle should check that SplitStyleAbovePoint returns node.

No regressions from this change, let's uplift to ESR52.3
Attachment #8871627 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
(In reply to Makoto Kato [:m_kato] from comment #11)
> [Is this code covered by automated tests?]:
> No, I cannot create automated test.  But crash reason is clear.
> 
> [Has the fix been verified in Nightly?]:
> I cannot reproduce this.
> 
> [Needs manual test from QE? If yes, steps to reproduce]:
> No

Setting qe-verify- based on Makoto Kato's assessment on manual testing needs.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.