615977 - Make nsCSSValue::BufferFromString() return an already_AddRefed pointer

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

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)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix

Comment 2

[Daniel Holbert [:dholbert]]

(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)

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 4

[Daniel Holbert [:dholbert]]

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.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v2 (review comment addressed) [r=dbaron]

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.

Comment 6

[Daniel Holbert [:dholbert]]

> 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

Comment 7

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/mozilla-central/rev/8c3e817ba034