bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

UMR [@ nsRange::SetEnd] from alloc [@ nsHTMLEditor::SplitStyleAbovePoint]

RESOLVED FIXED in mozilla1.9.3a4

Status

()

Core
Editor
--
major
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: mats)

Tracking

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

Trunk
mozilla1.9.3a4
testcase, valgrind
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking2.0 -)

Details

(Whiteboard: [sg:low?])

Attachments

(4 attachments)

(Reporter)

Description

9 years ago
Created attachment 422685 [details]
testcase

To reproduce:

valgrind --dsymutil=yes --track-origins=yes ~/central/debug-obj/dist/MinefieldDebug.app/Contents/MacOS/firefox-bin f.html

Result:

==967== Conditional jump or move depends on uninitialised value(s)
==967==    at 0x3619DC2: nsRange::SetEnd(nsINode*, int) (nsRange.cpp:699)
==967==    by 0x33AE70D: nsTypedSelection::Collapse(nsINode*, int) (nsSelection.cpp:4864)
==967==    by 0x33AE9B9: nsTypedSelection::Collapse(nsIDOMNode*, int) (nsSelection.cpp:4837)
==967==    by 0x3C83D95: nsHTMLEditRules::CreateStyleForInsertText(nsISelection*, nsIDOMDocument*) (nsHTMLEditRules.cpp:4564)
==967==    by 0x3C88F58: nsHTMLEditRules::WillInsert(nsISelection*, int*) (nsHTMLEditRules.cpp:1333)
==967==    by 0x3C946CD: nsHTMLEditRules::WillDoAction(nsISelection*, nsRulesInfo*, int*, int*) (nsHTMLEditRules.cpp:686)
==967==    by 0x3C46F1A: nsHTMLEditor::InsertHTMLWithContext(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, nsIDOMDocument*, nsIDOMNode*, int, int) (nsHTMLDataTransfer.cpp:417)
==967==    by 0x3C3F83E: nsHTMLEditor::InsertHTML(nsAString_internal const&) (nsHTMLDataTransfer.cpp:256)
==967==    by 0x3EF5176: nsInsertHTMLCommand::DoCommandParams(char const*, nsICommandParams*, nsISupports*) (nsComposerCommands.cpp:1472)
==967==    by 0x3D6F2D0: nsControllerCommandTable::DoCommandParams(char const*, nsICommandParams*, nsISupports*) (nsControllerCommandTable.cpp:208)
==967==    by 0x3D696FC: nsBaseCommandController::DoCommandWithParams(char const*, nsICommandParams*) (nsBaseCommandController.cpp:185)
==967==    by 0x3D6C781: nsCommandManager::DoCommand(char const*, nsICommandParams*, nsIDOMWindow*) (nsCommandManager.cpp:271)
==967==  Uninitialised value was created by a stack allocation
==967==    at 0x3C6CD7A: nsHTMLEditor::SplitStyleAbovePoint(nsCOMPtr<nsIDOMNode>*, int*, nsIAtom*, nsAString_internal const*, nsCOMPtr<nsIDOMNode>*, nsCOMPtr<nsIDOMNode>*) (nsHTMLEditorStyle.cpp:583)
(Reporter)

Updated

9 years ago
Duplicate of this bug: 547367
(Reporter)

Comment 2

9 years ago
SplitStyleAbovePoint calls SplitNodeDeep without checking rv.  SplitNodeDeep fails and doesn't set its out-param, but SplitStyleAbovePoint assumes it set its out-param.
blocking2.0: --- → ?
(Reporter)

Comment 3

9 years ago
Created attachment 427901 [details] [diff] [review]
possible fix

This quiets Valgrind (tested with the dup's testcase).  I didn't even try to understand the surrounding code enough to tell whether adding an early return here can create new problems.
(Assignee)

Comment 4

9 years ago
Created attachment 428121 [details] [diff] [review]
fix 2

Jesse's patch makes sense since SplitNodeDeep() could fail for any
number of reasons (so I included it here).
For the testcase though, I don't think it makes sense to call
SplitStyleAbovePoint() on the root node.  This patch gives the
resulting DOM <html>foo<head>...
Assignee: nobody → matspal
Attachment #428121 - Flags: review?(peterv)
(Assignee)

Updated

9 years ago
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 428121 [details] [diff] [review]
fix 2

>diff --git a/editor/libeditor/html/nsHTMLEditorStyle.cpp b/editor/libeditor/html/nsHTMLEditorStyle.cpp

>+      nsresult rv = SplitNodeDeep(tmp, *aNode, *aOffset, &offset, PR_FALSE, outLeftNode, outRightNode);

Long line.
We probably also have a bug where SplitStyleAbovePoint might split nodes that are not editable? :-/
Attachment #428121 - Flags: review?(peterv) → review+
(Assignee)

Comment 6

8 years ago
Created attachment 432674 [details] [diff] [review]
mochitest.diff
(Assignee)

Comment 7

8 years ago
Landed with nit fixed, but without the mochitest for now:
http://hg.mozilla.org/mozilla-central/rev/a20ed5e85c3d
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
(Assignee)

Comment 8

8 years ago
(In reply to comment #5)
> We probably also have a bug where SplitStyleAbovePoint might split nodes that
> are not editable? :-/

I couldn't find a bug, so I filed bug 552610.
Group: core-security
You need to log in before you can comment on or make changes to this bug.