864236 - Make multi-MiB allocations in the url-classifier fallible

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Nicholas Nethercote [inactive]]

In bug 862592 I found that the url-classifier is doing frequent multi-MiB allocations.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Make multi-MiB allocations in the url-classifier fallible.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 740178 [details] [diff] [review]
Make multi-MiB allocations in the url-classifier fallible.

The change in itself is OK, but you must improve the error handling if you do this. If you look at the callers, they will reset (=delete) the entire SafeBrowsing database if there is a failure updating it, because they presume this will only happen if it's corrupted (given that OOM was not possible before).

You probably want to change SUCCESS_OR_RESET so it does the right thing when your OOM error triggers (i.e. propagate failure upwards, don't delete the table).

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Make multi-MiB allocations in the url-classifier fallible.

This patch modifies SUCCESS_OR_RESET.  I don't understand this code at all, so
if this doesn't suffice perhaps you could take possession of this bug, gcp?

BTW, ReadTArray returns NS_ERROR_OF_OUT_MEMORY if the SetLength() call fails,
so there was already some "reset DB on OOM" behaviour here, AFAICT.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 740184 [details] [diff] [review]
Make multi-MiB allocations in the url-classifier fallible.

>BTW, ReadTArray returns NS_ERROR_OF_OUT_MEMORY if the SetLength() call fails,
>so there was already some "reset DB on OOM" behaviour here, AFAICT.

Does SetLength return an error on OOM or does it crash? Some of this code has gone back and forth between being fallible and not. Crashing during an update is acceptable - the code deals with it.

It looks like Classifier::ApplyUpdates needs a very similar fix on line 303, other than that I think this is fine. Possibly. r+ assuming you'll fix that one too.

>I don't understand this code at all, so if this doesn't suffice perhaps you could take possession of this bug, gcp?

That's a poor cop-out :) I can take possesion but that would be the equivalent of "postpone fixing this indefinitely". I'd rather have you fix it and have at least one other person with halfway of an understanding of this code.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Make some allocations fallible and deal with the error

Fixed up the things I noticed. This could use a lookover to see if there's any nasty cases I missed.

Comment 6

[Dave Camp (:dcamp)]

Comment on attachment 779655 [details] [diff] [review]
Patch 1. Make some allocations fallible and deal with the error

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

Looks good to me.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

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

Comment 8

[Nicholas Nethercote [inactive]]

Thanks for pushing this through, gcp!

Comment 9

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

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