crash [@ nsHTMLEditor::GetInlinePropertyBase] if !aAttribute

RESOLVED FIXED in mozilla8

Status

()

Core
Editor
--
critical
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: timeless, Assigned: kaze)

Tracking

({coverity, crash})

Trunk
mozilla8
coverity, crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [post-2.0], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
998 nsHTMLEditor::GetInlinePropertyBase(nsIAtom *aProperty, 
999                              const nsAString *aAttribute,
1000                              const nsAString *aValue,

1037   if ((NS_SUCCEEDED(result)) && currentItem)
1042     if (isCollapsed)

1047       if (aAttribute)
1048       {
1049         nsString tString(*aAttribute); //MJUDGE SCC NEED HELP

1060       if (isSet) 
1061       {
1062         *aFirst = *aAny = *aAll = theSetting;
1063         return NS_OK;

1065       if (!useCSS) {
1067         IsTextPropertySetByContent(collapsedNode, aProperty, aAttribute, aValue,
1068                                    isSet, getter_AddRefs(resultNode), outValue);

!isSet seems useless given 1063
1071         if (!isSet && aCheckDefaults) 

1076           if (TypeInState::FindPropInList(aProperty, *aAttribute, outValue, mDefaultStyles, index))
(Reporter)

Comment 1

7 years ago
ok, so the isSet check is needed because:

1067         IsTextPropertySetByContent(collapsedNode, aProperty, aAttribute, aValue,
1068                                    isSet, getter_AddRefs(resultNode), outValue);

mutates isSet
(Reporter)

Comment 2

7 years ago
fwiw, i think i've tried fighting this stuff every couple of years, the code should really have been moved to using nsAString& instead of nsAString*, and it could use Voidable strings or something if it needed to.

From memory, there's one or maybe two paths which actually do deal in null aAttributes, and most other paths don't think they'll send in null pointers. But proving this is really hard.
Assignee: nobody → ehsan
Whiteboard: [post-2.0]
Crash Signature: [@ nsHTMLEditor::GetInlinePropertyBase]
(In reply to comment #2)
> fwiw, i think i've tried fighting this stuff every couple of years, the code
> should really have been moved to using nsAString& instead of nsAString*, and
> it could use Voidable strings or something if it needed to.

This is a good idea, we should do this.
Assignee: ehsan → kaze
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

6 years ago
Created attachment 548593 [details] [diff] [review]
patch proposal
Attachment #548593 - Flags: review?(ehsan)
Attachment #548593 - Flags: review?(ehsan) → review+
http://hg.mozilla.org/mozilla-central/rev/f312978b8b05
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.