Closed
Bug 615977
Opened 14 years ago
Closed 14 years ago
Make nsCSSValue::BufferFromString() return an already_AddRefed pointer
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(2 files)
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dholbert
:
review+
roc
:
approval2.0+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•14 years ago
|
||
Attachment #494488 -
Flags: review?(dbaron)
Assignee | ||
Comment 2•14 years ago
|
||
(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
Assignee | ||
Updated•14 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Comment 3•14 years ago
|
||
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+
Assignee | ||
Comment 4•14 years ago
|
||
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.
Assignee | ||
Comment 5•14 years ago
|
||
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?
Assignee | ||
Updated•14 years ago
|
Attachment #494819 -
Attachment description: fix v2 (review comment addressed) → fix v2 (review comment addressed) [r=dbaron]
Attachment #494819 -
Flags: review+
Assignee | ||
Comment 6•14 years ago
|
||
> 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
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs approval]
Attachment #494819 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs approval] → [needs landing]
Comment 7•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
You need to log in
before you can comment on or make changes to this bug.
Description
•