962608 - Make PL_DHashTableInit infallible by default

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

I think there's some other way to trigger a crash that makes it show up like an OOM in the crash stack, so we should use that.

Comment 1

[Benjamin Smedberg]

Are you talking about a specific caller of PL_DHashTableInit?

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Add an infallible version of PL_DHashTableInit.

This is what I had in mind.  There are three callers of Init that crash on failure.  The idea
of InitInfallible (naming suggestions welcome) is to crash in a more informative way.  If the
arguments are bogusly large, then just MOZ_CRASH(), otherwise do NS_ABORT_OOM() with the size
we attempted to allocate. Two of the callers do the latter all the time, which is potentially
wrong. The CC just always does MOZ_CRASH(), which makes the crash signature in bug 958091
somewhat obscure.

All of the other callers of Init at least nominally try to handle the failure in a graceful
manner, or ignore the return value, so I left them alone.

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 8363725 [details] [diff] [review]
Add an infallible version of PL_DHashTableInit.

green L64 debug try run: https://tbpl.mozilla.org/?tree=Try&rev=404053c2911c

Comment 4

[Benjamin Smedberg]

Comment on attachment 8363725 [details] [diff] [review]
Add an infallible version of PL_DHashTableInit.

Normally for XPCOM functions I have made the default function infallible, and the fallible version has the same name but you pass a fallible_t() to it, example at http://hg.mozilla.org/mozilla-central/annotate/9db7cf4cc385/xpcom/string/public/nsTSubstring.h#l340

So if this were normal C++ code, I'd prefer that naming strategy. But since this is still pretty C-ish code that we only happen to compile as C++, this version is ok. I'd be happy if you did it the other way also. r=me either way.

Comment 5

[Andrew McCreight [:mccr8]]

Oh, that's a good point.  I always forget about that for some reason.  I'll take a look at it after the DOM work week is over.

Comment 6

[Andrew McCreight [:mccr8]]

The drawback of making this function infallible by default is that there are about 60 places it is called, so to preserve the current behavior I'd have to change a lot of places.  Of course, in many of these places it is only allocating a very tiny hashtable of 16 entries, so having it be infallible is probably better, but since they still check the result (like this: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentList.cpp#221 ) I'll have to change the code either way.  I'll think about it some more.  Any patch that instead uses fallible_t() will be pretty giant, and I'd ask for review from maybe nfroyd on it...

Comment 7

[Andrew McCreight [:mccr8]]

bsmedberg, what do you think about broadening the scope of this bug to making PL_DHashTableInit by default?  Most of the calls ask for a size of 16, so it should be okay to just make them infallible.  (I have a patch already written.)

Unfortunately, I can't do the fallible_t() thing because the declarations are inside an extern "C" block, which disallows any overloading.  But adding a function PL_DHashTableInitFallible doesn't seem too bad.  Clang 3.3 gives an incorrect error message when you do that, by the way. I should file a bug on that.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: Make PL_DHashTableInit infallible by default.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: Make PL_DHashTableInit infallible by default.

Comment 10

[Benjamin Smedberg]

I'm fine with making it infallible by default if there aren't any callers who can be affected by content-controlled sizes. Also I'm fine with removing the extern "C" which is ancient and I hope unneeded.

Comment 11

[Andrew McCreight [:mccr8]]

Yeah, any caller where there isn't a hard coded constant size of <= 20, that doesn't already just crash on failure, I just converted to be fallible, so it should be ok.  Thanks!

Comment 12

[Andrew McCreight [:mccr8]]

There are a couple of uses in comm-central I should convert, too:
  https://mxr.mozilla.org/comm-central/search?string=PL_DHashTableInit

Comment 13

[Andrew McCreight [:mccr8]]

So, do you have any thoughts on how I should review this patch, Nathan?  I could just get you to review it all, or I could split it up and get various module peers to review it.  There's nothing super amazing going on, but maybe it would be a good idea?  (I have it split up a bit locally.)

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #13)
> So, do you have any thoughts on how I should review this patch, Nathan?  I
> could just get you to review it all, or I could split it up and get various
> module peers to review it.  There's nothing super amazing going on, but
> maybe it would be a good idea?  (I have it split up a bit locally.)

Glancing through, it seems like getting module owners to review appropriately would be good.  Some initializations have been translated to fallible defaults, some not, and I'm not the right person to judge such changes across a wide variety of modules.  I have not acquired sufficient Ehsan-fu to review such things.

You can flip a coin as to whether Benjamin or myself get to review the XPCOM changes. :)

Comment 15

[Andrew McCreight [:mccr8]]

Attachment: Make PL_DHashTableInit by default (XPCOM changes).

I'll wait until this is r+ed before uploading the rest of the chunks, but you can mostly see the changes in the other patch I already uploaded.  Though there are a few minor changes from there.  I'll land it with all of the patches mashed together.

I initially made nsHashtable's constructor take an argument that let you decide if it is infallible or not, but it didn't really seem worth the complexity, so it is now always fallible, preserving existing behavior.

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 8379145 [details] [diff] [review]
Make PL_DHashTableInit by default (XPCOM changes).

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

r=me with the changes below.

::: xpcom/ds/nsPersistentProperties.cpp
@@ +461,2 @@
>  nsPersistentProperties::Init()
>  {

Can you file a followup bug about subsuming this function into the constructor?

::: xpcom/glue/pldhash.h
@@ +399,5 @@
>  /*
>   * Initialize table with ops, data, entrySize, and capacity.  Capacity is a
>   * guess for the smallest table size at which the table will usually be less
>   * than 75% loaded (the table will grow or shrink as needed; capacity serves
>   * only to avoid inevitable early growth from PL_DHASH_MIN_SIZE).

Can you add a blurb here about crashing if we can't allocate appropriate amounts of memory?

@@ +406,5 @@
>  PL_DHashTableInit(PLDHashTable *table, const PLDHashTableOps *ops, void *data,
>                    uint32_t entrySize, uint32_t capacity);
>  
> +NS_COM_GLUE bool
> +PL_DHashTableInit(PLDHashTable *table, const PLDHashTableOps *ops, void *data,

Can you add a comment here describing the function?

::: xpcom/tests/TestPLDHash.cpp
@@ -87,5 @@
>  
>    PLDHashTable t;
> -  bool ok = PL_DHashTableInit(&t, &ops, nullptr, sizeof(PLDHashEntryStub), 256);
> -  if (!ok)
> -    return false;

I guess the logic here is that if we're failing on such a small allocation, it's better to abort the test rather than try to continue?

I'm inclined to just keep this one fallible so that we get nicer error messages.

Comment 17

[Andrew McCreight [:mccr8]]

(In reply to Nathan Froyd (:froydnj) from comment #16)
> Can you file a followup bug about subsuming this function into the constructor?

Good idea. Filed bug 975037.

I'll update the comments like you suggest.

> >    PLDHashTable t;
> > -  bool ok = PL_DHashTableInit(&t, &ops, nullptr, sizeof(PLDHashEntryStub), 256);
> > -  if (!ok)
> > -    return false;
> 
> I guess the logic here is that if we're failing on such a small allocation, it's better to abort the test rather than try to continue?

Well, my idea was really just to test the infallible code in at least one place, though I guess we're not really going to call the extra code, so it doesn't matter much.  I can keep it fallible.

Comment 18

[Andrew McCreight [:mccr8]]

Attachment: Convert DOM and CC code to infallible PL_DHashTableInit.

GLOBALNAME_HASHTABLE_INITIAL_SIZE is 1024 so I left that one infallible.

Comment 19

[Andrew McCreight [:mccr8]]

Attachment: Convert chrome/, RDF, plugin, prefapi and nsHTMLEntities to infallible PL_DHashTableInit.

PREF_HASHTABLE_INITIAL_SIZE is 2048.

NS_HTML_ENTITY_COUNT is around 200.

Note that the nsPluginStreamListenerPeer code is somewhat bogus, as it should really check if the mHashtable.ops is null. I can file a followup for that, but my patch here does not change that behavior. (I left nsHashtable as fallible, because otherwise you have to pass a flag in for falliblity, because sometimes a large value can get passed in, when we clone the table, and this is deprecated code so it didn't seem worth making it more complicated.)

Comment 20

[Andrew McCreight [:mccr8]]

Attachment: Layout changes for infallible PL_DHashTableInit.

I'm not sure who should review these changes.  Feel free to bounce this to somebody else.

64 isn't very big, so I make the Init in nsCSSRuleProcessor to be infallible if you prefer. I was just erring on the side of being conservative.

Comment 21

[Andrew McCreight [:mccr8]]

Attachment: Networking changes for infallible PL_DHashTableInit.

Feel free to forward this to a more appropriate reviewer as necessary.

NUM_HTTP_ATOMS is around 100.

I made two of these fallible, as they are not minimum-size hashtables, but I could make them infallible if you think that makes more sense.

(As a side note, the Init in nsDiskCacheBinding actually creates a hashtable of size 16, as that's the minimum size, but I just left the 0 as is.)

Comment 22

[Andrew McCreight [:mccr8]]

Attachment: Convert misc. code to using infallible PL_DHashTableInit. r=ehsan,jfkthame,briansmith

These are all very small changes.

The minimum hashtable size is actually 16, but I just left the 4 and 0 Inits alone, rather than change them to PL_DHASH_MIN_SIZE.

r?ehsan for nsCommandParams.cpp.
r?jfkthame for gfxFT2FontList.cpp.
r?briansmith for nsCertTree.cpp.  I could make this infallible if you think that is better.  128 seems pretty small, but I was erring on the side of being conservative.

Comment 23

[Jonathan Kew [:jfkthame]]

Comment on attachment 8379175 [details] [diff] [review]
Convert misc. code to using infallible PL_DHashTableInit.  r=ehsan,jfkthame,briansmith

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

r=me for the gfxFT2FontList change.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8379171 [details] [diff] [review]
Layout changes for infallible PL_DHashTableInit.

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

Looks good.

I presume all these should really be converted to use nsTHashtable eventually.

Comment 25

[Andrew McCreight [:mccr8]]

> I presume all these should really be converted to use nsTHashtable eventually.
Probably, but that's not going to happen until we get rid of nsHashtable (bug 971264).

Comment 26

[Jason Duell]

Comment on attachment 8379172 [details] [diff] [review]
Networking changes for infallible PL_DHashTableInit.

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

Jolly good stuff.

Comment 27

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c95f331f043d

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c95f331f043d

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Bug 975029 landed, so I guess we can close this now?