Remove OOM handling code for CounterListFor in nsCounterManager::AddResetOrIncrement

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: mccr8, Assigned: nl, Mentored)

Tracking

(Blocks 1 bug, {coverity})

Trunk
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [CID 221146])

Attachments

(2 attachments, 1 obsolete attachment)

No description provided.
We should probably just remove the OOM-handling code now that we have infallible malloc.
Whiteboard: [CID 221146][MemShrink] → [CID 221146]
Mentor: continuation
Keywords: mlk
Summary: nsCounterManager::AddResetOrIncrement leaks |node| when |counterList| is null → Remove OOM handling code for CounterListFor in nsCounterManager::AddResetOrIncrement
Hi, what parts of the codebase are to be deleted?
What needs to be done is to check the places that call nsCounterManager::CounterListFor, and remove any null checks they do on the return value.  It used to be that allocation could fail and we'd return null, but now the whole browser will just crash if the allocation fails (which is very unlikely) so that null check isn't needed.  The comment describing CounterListFor() by its declaration also needs to be updated.

This should include all of the places that call it:
  http://mxr.mozilla.org/mozilla-central/ident?i=CounterListFor
Attaching a patch for this task.

Should i run some subset of try tests for this fix? If yes - what subset of tests you would suggest.
Assignee: nobody → nicklebedev37
Status: NEW → ASSIGNED
Attachment #8487846 - Flags: review?(continuation)
Comment on attachment 8487846 [details] [diff] [review]
Remove OOM null checks of CounterListFor return result

Review of attachment 8487846 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me.  I don't think a try run is really needed.  Just make sure you can use the browser for a minute.

I'll forward this review request to a layout peer.
Attachment #8487846 - Flags: review?(dholbert)
Attachment #8487846 - Flags: review?(continuation)
Attachment #8487846 - Flags: feedback+
Comment on attachment 8487846 [details] [diff] [review]
Remove OOM null checks of CounterListFor return result

Review of attachment 8487846 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this!  r=me with the following addressed:

::: layout/base/nsCounterManager.h
@@ +25,5 @@
>          RESET,     // a "counter number" pair in 'counter-reset'
>          INCREMENT, // a "counter number" pair in 'counter-increment'
>          USE        // counter() or counters() in 'content'
>      };
> +

Nit: it looks like over half of the changes in this patch are actually whitespace fixes (and aren't adjacent to the code that's actually changing -- they're just somewhere in the same file).

I'm guessing these were done by an overzealous editor extension or hg extension that you happen to be using, which fixes up all whitespace issues in any file that you touch.

While it's good to fix these (death to end-of-line-whitespace!), it's generally best not to mix them in with actual functional changes, in a patch, unless it's part of the actual code that's being rewritten.  If they're all mixed together, it makes it harder to skim a patch and quickly see what's actually changing. (both for the reviewer, and for all the hg archeologists in the future)  

So: for the benefit of those future archeologists, and for hg hygiene, could you split the whitespace changes into their own patch?  (I'm happy to have the whitespace-cleanup patch land as a followup here (or a "part 0"); no need to give it its own bug or anything.  It's just good hygiene to make the actual functional patch nice and focused.)

@@ +226,5 @@
>      bool AddCounterResetsAndIncrements(nsIFrame *aFrame);
>  
>      // Gets the appropriate counter list, creating it if necessary.
> +    // It's ok not to make null check on return result since this method
> +    // uses infallible memory allocation now.

Replace this comment with the following, or something like it:
    // Guaranteed to return non-null. (Uses an infallible hashtable API.)

This is more concise and to the point, and also technically we don't use "infallible allocation" here, I think.  As far as I can tell, we use "Put" from nsBaseHashtable.h, which asks for *fallible* allocation, but then it aborts if the alloc fails.  So the effect is the same as if we had infallible allocation.  Anyway, kind of a subtle point, but might as well generalize the language to make it more correct. :))
Attachment #8487846 - Flags: review?(dholbert) → review+
Attachment #8487846 - Attachment is obsolete: true
Attachment #8488500 - Flags: review+
Attachment #8488501 - Flags: review+ → review?(dholbert)
Attachment #8488500 - Flags: review+ → review?(dholbert)
Attachment #8488500 - Flags: review?(dholbert) → review+
Comment on attachment 8488501 [details] [diff] [review]
Part 1. Remove OOM null checks of CounterListFor return result.

Review of attachment 8488501 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! r=me
Attachment #8488501 - Flags: review?(dholbert) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/009f156e7319
https://hg.mozilla.org/mozilla-central/rev/1691dce8634d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.