Last Comment Bug 649163 - nsDOMCSSDeclaration::GetCSSParsingEnvironment should not AddRef its output
: nsDOMCSSDeclaration::GetCSSParsingEnvironment should not AddRef its output
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla6
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-11 15:53 PDT by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2011-04-27 18:19 PDT (History)
1 user (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Stop refcounting the return values from GetCSSParsingEnvironment, where possible. (19.43 KB, patch)
2011-04-27 09:38 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
dbaron: review+
Details | Diff | Review
With that change reverted (18.83 KB, patch)
2011-04-27 13:54 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-11 15:53:27 PDT
Filed per request in bug 435442 comment 37:

nsDOMCSSDeclaration::GetCSSParsingEnvironment (a pure virtual function) and its implementations currently AddRef all the out-parameters.  However, this is probably unnecessary, and it should be changed to fill in weak pointers.
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-04-27 09:38:02 PDT
Created attachment 528613 [details] [diff] [review]
Stop refcounting the return values from GetCSSParsingEnvironment, where possible.
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-27 13:08:39 PDT
Comment on attachment 528613 [details] [diff] [review]
Stop refcounting the return values from GetCSSParsingEnvironment, where possible.

>@@ -120,17 +120,17 @@ nsDOMCSSAttributeDeclaration::DocToUpdat
>...
>   // We need GetOwnerDoc() rather than GetCurrentDoc() because it might
>   // be the BeginUpdate call that inserts mElement into the document.
>-  return mElement->GetOwnerDoc();
>+  return mElement->GetCurrentDoc();
> }

Were you just experimenting with something here?  I'm guessing this
wasn't intended to be part of the patch.  If it was, please explain.

r=dbaron assuming that wasn't supposed to be here
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-04-27 13:12:29 PDT
> Were you just experimenting with something here?

Yes.  I was sure I'd reverted that change, but clearly not.  Thank you for catching it!

Reverted locally.
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-04-27 13:54:08 PDT
Created attachment 528705 [details] [diff] [review]
With that change reverted
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-04-27 18:19:02 PDT
This was pushed as http://hg.mozilla.org/mozilla-central/rev/27987cc4f4bb

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