Closed Bug 615977 Opened 9 years ago Closed 9 years ago

Make nsCSSValue::BufferFromString() return an already_AddRefed pointer

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(2 files)

Right now we have this documentation for nsCSSValue::BufferFromString:

>   // Returns an already addrefed buffer.  Can return null on allocation
>   // failure.
>   static nsStringBuffer* BufferFromString(const nsString& aValue);

Filing this bug on returning an already_AddRefed pointer instead, to make the "already addrefed" assumption more explicit to clients of the method, and to allow easier capturing of the returned value in e.g. a nsRefPtr. (so clients don't have to remember to explicitly release the value)
Attached patch fixSplinter Review
Attachment #494488 - Flags: review?(dbaron)
(BTW: I noticed the need for this bug when looking at birtles' patch for Bug 607537, which calls BufferFromString.  That patch currently has to explicitly wrap the return value with "getter_AddRefs" to avoid double-addref'ing)
Blocks: 607537
OS: Linux → All
Hardware: x86_64 → All
Comment on attachment 494488 [details] [diff] [review]
fix

>diff --git a/content/html/content/src/nsGenericHTMLElement.cpp b/content/html/content/src/nsGenericHTMLElement.cpp
>-          if (NS_LIKELY(buffer != 0)) {
>+          if (NS_LIKELY(!buffer)) {

>-            if (NS_LIKELY(img != 0)) {
>+            if (NS_LIKELY(!img)) {

You made both of these backwards.  Please fix and add a test if there isn't one already.

r=dbaron with that
Attachment #494488 - Flags: review?(dbaron) → review+
oops, thanks! :)

I'll make sure there are tests that catch that. There probably are -- I didn't test this so far, beyond making sure it compiles.
Here's the patch with the logic fixed per comment 3.

I pushed the old fix + this fix to try-server (debug-only/linux-only for minimizing resource use), to make sure that the old fix fails some tests.

Requesting approval. Justification: minimal change, with no expected perf or behavior impact.  Just making refcounting assumptions more explicit in this method (nsCSSValue::BufferFromString) & its callers.
Attachment #494819 - Flags: approval2.0?
Attachment #494819 - Attachment description: fix v2 (review comment addressed) → fix v2 (review comment addressed) [r=dbaron]
Attachment #494819 - Flags: review+
> You made both of these backwards.  Please fix and add a test if there isn't one
> already.
Good news - the old patch fails test_unsecureBackground.html. I gave it another TryServer run to confirm, and it failed in exactly the same way again. There are no bugs open about that test, and the logic reversal was in background-related code, so I don't think it was just an unlucky intermittent thing.

So the first patch-version's logic-reversal bug does have (some) test coverage.  The failures are:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1291379471.1291380354.18373.gz#err0
Whiteboard: [needs approval]
Whiteboard: [needs approval] → [needs landing]
Pushed http://hg.mozilla.org/mozilla-central/rev/8c3e817ba034
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
You need to log in before you can comment on or make changes to this bug.