Closed Bug 848895 Opened 12 years ago Closed 12 years ago

Hold strong refs to editrules when calling into it, and always null-check the editor in editrules code

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla22
Tracking Flags:
Tracking Status
firefox21 --- wontfix
firefox22 --- fixed
firefox23 --- fixed
firefox-esr17 --- wontfix
b2g18 --- wontfix

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

(Keywords: sec-want, Whiteboard: [adv-main22-])

Attachments

(2 files, 1 obsolete file)

part 1. Always hold an on-stack strong reference to mRules when calling into it.
(deleted), patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
part 2. Null-check the pointer to the editor that the edit rules hold.
(deleted), patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
See bug 848644 for discussion.
Attached patch Partial part 2 (obsolete) (deleted) — — Splinter Review
Up to about line 2878 of nsHTMLEditRules.cpp so far. Lots more to go
Whiteboard: [need to finish up patch]
Marking sec-other because this is more of a preventative measure, as far as I understand.
Keywords: sec-other, sec-want
Attachment #722394 - Flags: review?(ehsan) → review+
This may be the least reviewable patch I have ever written....
Attachment #725260 - Flags: review?(ehsan)
Attachment #722396 - Attachment is obsolete: true
Comment on attachment 725260 [details] [diff] [review] part 2. Null-check the pointer to the editor that the edit rules hold. Review of attachment 725260 [details] [diff] [review]: ----------------------------------------------------------------- This patch makes me so sad. Thanks for writing it! ;-) ::: editor/libeditor/html/nsHTMLEditRules.cpp @@ +807,5 @@ > > int32_t offset, rootOffset; > nsCOMPtr<nsIDOMNode> parent = nsEditor::GetNodeLocation(rootElem, &rootOffset); > + NS_ENSURE_STATE(mHTMLEditor); > + res = mHTMLEditor->GetStartNodeAndOffset(selection, getter_AddRefs(parent), &offset); I assume you changed the indentation here by mistake. @@ +6056,5 @@ > for (i=listCount-1; i>=0; i--) > { > nsCOMPtr<nsIDOMNode> node = outArrayOfNodes[i]; > if (!aDontTouchContent && IsInlineNode(node) > + && Nit: please put this at the end of previous line.
Attachment #725260 - Flags: review?(ehsan) → review+
> I assume you changed the indentation here by mistake. Yes, copy/paste brought along some spaces. > Nit: please put this at the end of previous line. Done. https://hg.mozilla.org/integration/mozilla-inbound/rev/7437279a223d https://hg.mozilla.org/integration/mozilla-inbound/rev/4c2a4448bd09
Flags: in-testsuite-
Whiteboard: [need to finish up patch]
Target Milestone: --- → mozilla22
Whiteboard: [adv-main22-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: