1024808 - Remove OOM handling code for CounterListFor in nsCounterManager::AddResetOrIncrement

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

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

We should probably just remove the OOM-handling code now that we have infallible malloc.

Comment 2

[Shashank VRSN Sabniveesu]

Hi, what parts of the codebase are to be deleted?

Comment 3

[Andrew McCreight [:mccr8]]

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

Comment 4

[Nick Lebedev [:nl]]

Attachment: Remove OOM null checks of CounterListFor return result

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.

Comment 5

[Andrew McCreight [:mccr8]]

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.

Comment 6

[Daniel Holbert [:dholbert]]

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. :))

Comment 7

[Nick Lebedev [:nl]]

Attachment: Part 0. Clean up trailing whitespaces.

Comment 8

[Nick Lebedev [:nl]]

Attachment: Part 1. Remove OOM null checks of CounterListFor return result.

Comment 9

[Daniel Holbert [:dholbert]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/009f156e7319
https://hg.mozilla.org/integration/mozilla-inbound/rev/1691dce8634d

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/009f156e7319
https://hg.mozilla.org/mozilla-central/rev/1691dce8634d