943021 - Safebrowsing service should use fallible arrays for its buffers

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Benjamin Smedberg]

https://crash-stats.mozilla.com/report/index/8a80fbdb-36d1-4991-9de3-761012131122

mozilla::safebrowsing::HashStore::ReadAddPrefixes() is calling
nsTArray_Impl<mozilla::safebrowsing::AddPrefix,nsTArrayInfallibleAllocator>::SetCapacity(unsigned int)

The various arrays in safebrowsing should use the fallible-type tarray, by modifying the typedefs at toolkit/components/url-classifier/Entries.h#243 and then checking the rest of the code for safety and error propagation.

I think this is the right component for safebrowsing...

Comment 1

[Peiyong Lin[:lpy](UTC+8)]

Hi. I was confused about the error propagation after changed |nsTArray| to |FallibleTArray|.

Comment 2

[Peiyong Lin[:lpy](UTC+8)]

oops, |NS_ERROR_OUT_OF_MEMORY| may be the right thing I need

Comment 3

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug943021.patch

Comment 4

[Gian-Carlo Pascutto [:gcp]]

I'll take a look at this when I get back from holidays (2 Jan).

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8351535 [details] [diff] [review]
bug943021.patch

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

A lot of this code is just duplicating things that were templated to nsTArray<T> to FallibleTArray<T>. I think this is avoidable by just switching the original implementation to the common base class. See this comment in nsTArray.h:

http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#722

So, I think you can get rid of the code duplication here, and just need to change the signatures from nsTArray<T> to nsTArray_Impl<T>.

::: toolkit/components/url-classifier/HashStore.cpp
@@ +579,1 @@
>    outBuff.SetLength(outsize);

This needs error checking, as it is now fallible.

@@ +715,5 @@
>  
>    nsresult rv = ByteSliceRead(mInputStream, &chunks, count);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  bool success = mAddPrefixes.SetCapacity(count);

You can just do if (!mAddPrefixes.SetCapacity(count)) here, the extra variable doesn't really aid readability or debugging.

Comment 6

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug943021.patch

Thank you :)

Comment 7

[Peiyong Lin[:lpy](UTC+8)]

Thank you. I set the check in flag :)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/77d3d487c68d

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/fx-team/rev/c467462fae8e

https://tbpl.mozilla.org/php/getParsedLog.php?id=32580660&tree=Fx-Team

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Was debug-only bustage.

Comment 11

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug943021-V3.patch

Comment 12

[Gian-Carlo Pascutto [:gcp]]

Maybe we should do a run on the tryserver first to check if there's nothing else that was overlooked? If you don't have L1 hg access I can do that for you.

Comment 13

[Peiyong Lin[:lpy](UTC+8)]

OK. I will do that. Thank you :)

Comment 14

[Peiyong Lin[:lpy](UTC+8)]

I saw build busted on Windows 2012 x64 Debug, but I don't know why. Any idea please?
https://tbpl.mozilla.org/?tree=Try&rev=7b08ff453bc3

Comment 15

[Gian-Carlo Pascutto [:gcp]]

Looks like infrastructure to me. Note that we don't run x64 Windows tests on m-i and m-c anyway. Looks   like you're good to go.

Comment 16

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug943021-Final.patch

OK. I set the check in flag. Thank you :)

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ed22a1ba0e13

Comment 18

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

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

Comment 19

[Tracy Walker [:tracy]]

This affects 27, 28 and 29. Verified fixed on the later per crash stats.

Is this something that should be uplifted or left to ride the trains?

Comment 20

[Benjamin Smedberg]

I would like this to ride the trains. We're no worse off than we were, and while the risk is small it's not zero.

Comment 22

[u279076]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #20)
> I would like this to ride the trains. We're no worse off than we were, and
> while the risk is small it's not zero.

At what point would we re-evaluate this position? It's currently up 5 positions to #8 in Firefox 28.

Comment 23

[Gian-Carlo Pascutto [:gcp]]

>At what point would we re-evaluate this position? It's currently up 5 positions to #8 in Firefox 28.

I think that's more likely caused by other bugs causing us to leak memory, which then cause SafeBrowsing to crash when it does periodic updates. Uplifting this is unlikely to lower the total amount of crashes, just move around the code where they happen. That said this isn't a high risk patch.

Comment 24

[u279076]

I had a second look at this in terms of Crashes per Install over time and this looks less concerning than I originally thought.

Firefox 28 Beta:
 * 7 days before 2014-02-27: 1.040 crashes per install
 * 7 days before 2014-02-20: 1.036 crashes per install
 * 7 days before 2014-02-13: 1.057 crashes per install

It looks to me like the numbers are somewhat stable. This doesn't change the fact that it's a topcrash but does make me more inclined to agree with letting this ride the trains.

Benjamin, can you check this to make sure I'm right in my assumptions?

Comment 25

[Benjamin Smedberg]

Yeah, that data looks stable enough.

Comment 26

[Robert Kaiser]

Anthony, how did you calculate this? Did you use the "installations" measure on the Signature Summary (which only counts the different installations seen in crashes) or did you your ADI values, which are general and independent of the single crash?

Comment 27

[u279076]

(In reply to Robert Kaiser (:[e-mail]) from comment #26)
> Anthony, how did you calculate this?

This was based purely on the Crashes per Install section of the Signature Summary tab.