Closed Bug 620319 Opened 9 years ago Closed 9 years ago

crash [@ nsHTMLEditor::GetInlinePropertyBase] if !aAttribute

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: timeless, Assigned: kaze)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, crash, Whiteboard: [post-2.0])

Crash Data

Attachments

(1 file)

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))
ok, so the isSet check is needed because:

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

mutates isSet
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
Status: NEW → ASSIGNED
Attached patch patch proposalSplinter Review
Attachment #548593 - Flags: review?(ehsan)
Attachment #548593 - Flags: review?(ehsan) → review+
http://hg.mozilla.org/mozilla-central/rev/f312978b8b05
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.