Last Comment Bug 620319 - crash [@ nsHTMLEditor::GetInlinePropertyBase] if !aAttribute
: crash [@ nsHTMLEditor::GetInlinePropertyBase] if !aAttribute
Status: RESOLVED FIXED
[post-2.0]
: coverity, crash
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla8
Assigned To: Fabien Cazenave [:kaze]
:
: Makoto Kato [:m_kato]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-19 18:11 PST by timeless
Modified: 2011-07-27 03:43 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch proposal (1.30 KB, patch)
2011-07-26 14:40 PDT, Fabien Cazenave [:kaze]
ehsan: review+
Details | Diff | Splinter Review

Description timeless 2010-12-19 18:11:36 PST
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))
Comment 1 timeless 2010-12-20 15:20:37 PST
ok, so the isSet check is needed because:

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

mutates isSet
Comment 2 timeless 2010-12-20 23:50:19 PST
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.
Comment 3 :Ehsan Akhgari 2011-07-21 16:02:11 PDT
(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.
Comment 4 Fabien Cazenave [:kaze] 2011-07-26 14:40:05 PDT
Created attachment 548593 [details] [diff] [review]
patch proposal
Comment 5 Marco Bonardo [::mak] 2011-07-27 03:43:42 PDT
http://hg.mozilla.org/mozilla-central/rev/f312978b8b05

Note You need to log in before you can comment on or make changes to this bug.