Closed Bug 943021 Opened 6 years ago Closed 6 years ago

Safebrowsing service should use fallible arrays for its buffers

Categories

(Toolkit :: Safe Browsing, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox27 - wontfix
firefox28 - wontfix
firefox29 --- fixed

People

(Reporter: benjamin, Assigned: lpy)

References

(Blocks 1 open bug)

Details

(Whiteboard: [mentor=gcp][lang=c++][good first bug])

Crash Data

Attachments

(1 file, 3 obsolete files)

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...
Whiteboard: [mentor=gcp][lang=c++][good first bug]
Hi. I was confused about the error propagation after changed |nsTArray| to |FallibleTArray|.
Assignee: nobody → pylaurent1314
oops, |NS_ERROR_OUT_OF_MEMORY| may be the right thing I need
Attached patch bug943021.patch (obsolete) — Splinter Review
Attachment #8351535 - Flags: review?(gpascutto)
I'll take a look at this when I get back from holidays (2 Jan).
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.
Attachment #8351535 - Flags: review?(gpascutto) → review-
Attached patch bug943021.patch (obsolete) — Splinter Review
Thank you :)
Attachment #8351535 - Attachment is obsolete: true
Attachment #8355548 - Flags: review?(gpascutto)
Attachment #8355548 - Flags: review?(gpascutto) → review+
Thank you. I set the check in flag :)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/77d3d487c68d
Keywords: checkin-needed
Whiteboard: [mentor=gcp][lang=c++][good first bug] → [mentor=gcp][lang=c++][good first bug][fixed-in-fx-team]
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
Whiteboard: [mentor=gcp][lang=c++][good first bug][fixed-in-fx-team] → [mentor=gcp][lang=c++][good first bug]
Was debug-only bustage.
Attached patch bug943021-V3.patch (obsolete) — Splinter Review
Attachment #8355548 - Attachment is obsolete: true
Attachment #8356449 - Flags: review+
Keywords: checkin-needed
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.
Keywords: checkin-needed
OK. I will do that. Thank you :)
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
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.
OK. I set the check in flag. Thank you :)
Attachment #8356449 - Attachment is obsolete: true
Attachment #8357678 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/ed22a1ba0e13
Keywords: checkin-needed
Whiteboard: [mentor=gcp][lang=c++][good first bug] → [mentor=gcp][lang=c++][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ed22a1ba0e13
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=gcp][lang=c++][good first bug][fixed-in-fx-team] → [mentor=gcp][lang=c++][good first bug]
Target Milestone: --- → mozilla29
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?
Crash Signature: [@ mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::EnsureCapacity(unsigned int, unsigned int) | nsTArray_Impl<mozilla::safebrowsing::AddPrefix, ns…
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.
Duplicate of this bug: 970981
(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.
>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.
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?
Flags: needinfo?(benjamin)
Yeah, that data looks stable enough.
Flags: needinfo?(benjamin)
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?
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #26)
> Anthony, how did you calculate this?

This was based purely on the Crashes per Install section of the Signature Summary tab.
You need to log in before you can comment on or make changes to this bug.