1362761 - Crash in nsUrlClassifierPrefixSet::WritePrefixes

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-f718be0e-d495-495a-b8cf-73c490170506.
=============================================================
Crashing Thread (27)
Frame 	Module 	Signature 	Source
0 	XUL 	nsUrlClassifierPrefixSet::WritePrefixes(nsIOutputStream*) 	xpcom/glue/nsTArray.h:395
1 	XUL 	nsUrlClassifierPrefixSet::StoreToFile(nsIFile*) 	toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp:375
2 	XUL 	mozilla::safebrowsing::LookupCache::WriteFile() 	toolkit/components/url-classifier/LookupCache.cpp:154
3 	XUL 	mozilla::safebrowsing::Classifier::UpdateHashStore(nsTArray<mozilla::safebrowsing::TableUpdate*>*, nsACString_internal const&) 	toolkit/components/url-classifier/Classifier.cpp:998
4 	XUL 	mozilla::safebrowsing::Classifier::ApplyUpdates(nsTArray<mozilla::safebrowsing::TableUpdate*>*) 	toolkit/components/url-classifier/Classifier.cpp:532
5 	XUL 	nsUrlClassifierDBServiceWorker::FinishUpdate() 	toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:630
6 	XUL 	mozilla::detail::RunnableMethodImpl<nsresult (nsUrlClassifierDBServiceWorker::*)(), true, false>::Run 	xpcom/glue/nsThreadUtils.h:775
7 	XUL 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1216
8 	XUL 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp:361
9 	XUL 	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:368
10 	XUL 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:232
11 	XUL 	nsThread::ThreadFunc(void*) 	xpcom/threads/nsThread.cpp:467
12 	libnss3.dylib 	_pt_root 	nsprpub/pr/src/pthreads/ptthread.c:216
13 	libsystem_pthread.dylib 	_pthread_body 	
14 	libsystem_pthread.dylib 	_pthread_start 	
15 	libsystem_pthread.dylib 	thread_start 	
16 	libnss3.dylib 	libnss3.dylib@0x1b852f

this signature is regressing in firefox 52 and subsequent builds and primarily affects installations on android and macos. it looks like that may be related to bug 1305801.

Comment 1

[Nathan Froyd [:froydnj]]

[Tracking Requested - why for this release]: Lots of crashes on Android.  Might be worth including in a 53 dot release, not sure.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

100 or so crashes per week on release isn't so bad as to make me want to risk trying to fix this in 53. 

What would be good here is someone to investigate the crash and come up with a patch. Then we can think about uplift to 54. It is hard to tell if 55 is affected but I would assume it's likely.

Comment 3

[Dimi Lee [:dimi]]

his bug happens since 47,at least, I didn't dig further.

It looks like a regression of bug 1305801 because the function name changed from nsUrlClassifierPrefixSet::StoreToFile to nsUrlClassifierPrefixSet::WritePrefixes.

By using "nsUrlClassifierPrefixSet::StoreToFile" as the signature you will find the result[1] and look at the crash call stack it happened in the same place as this one[2].

[1] https://crash-stats.mozilla.com/signature/?signature=nsUrlClassifierPrefixSet%3A%3AStoreToFile&date=%3E%3D2016-01-01T01%3A11%3A00.000Z&date=%3C2016-11-30T01%3A11%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=version&_sort=-date&page=1
[2] https://crash-stats.mozilla.com/report/index/66f8a246-5a24-4aef-90bf-062362161128

Comment 4

[François Marier [:francois]]

As per comment 3, this is not a regression.

Comment 5

[Julien Cristau [:jcristau]]

Old crash, not a huge volume, doesn't seem worth tracking for 55.

Comment 6

[Thomas Nguyen (:tnguyen)]

I still could see it occurs on 56 and 57, give it a higher priority

Comment 7

[Dimi Lee [:dimi]]

In the latest crash report, mCanary.check added by thomas in Bug 1401066 doesn't intercept the error[1].
So this is not caused by memory pollution in "nsUrlClassifierPrefixSet" class.
Also from the crash dump, I don't think the call stack is polluted.

The problem seems related to |mIndexPrefixes| in nsUrlClassifierPrefixSet[1].
I found all those crash dumps will get the same |indexSize| and |indexDeltaSize|[2] and by analyzing crash dump we
can know that this is not a bug of update algorithm.

Also the |fileSize| returned by |CalculatePreallocateSize|[3] is wrong for all crash dumps.
|CalculatePreallocateSize| function is quite simple it only invovles |mTotalPrefixes| & |mIndexPrefixes|.

Since mTotalPrefixes is just an integer & mIndexPrefixes is a nsTArray, so I'll guess mIndexPrefixes is corrupted.
(Maybe also mIndexDeltas, if everything is corrupted then mCanary.check added by thomas should find this issue).

The roughly update flow invovles mIndexPrefixes is:
1. nsUrlClassifierPrefixSet::MakePrefixSet (set mIndexPrefixes)
2. nsUrlClassifierPrefixSet::CalculatePreallocateSize (use mIndexPrefixes)
3. nsUrlClassifierPrefixSet::WritePrefixes (use mIndexPrefixes).

In step2 the value is already wrong, so the incorrect result may come from |MakePrefixSet|.
By checking nsUrlClassifierPrefixSet::MakePrefixSet[4], the only thing I can think of to cause this problem is |Compact|. At the end of MakePrefixSet we call Compact of mIndexDeltas and mIndexPrefixes to minimize memory usage,
maybe the memory rearrangement causes some kind of problem.

Hi francois,
I don't have any evidence to prove |Compact| causes this issue, it's just a guess after analyzing the crash dump.
I am not sure if its a good idea but since we already fully enable V4 in nightly, so V2 will only be used in tracking protection.
Maybe we can remove "Compact" in Nightly build, it shouldn't affect performance too much because most cases are V4.

Then we can keep monitoring nightly build to see if this may be the root cause.

[1] https://crash-stats.mozilla.com/report/index/f157ed44-cc87-41b1-8636-ec5bf0171001
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierPrefixSet.h#78
[3] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp#503
[4] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp#380
[5] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp#90

Comment 8

[François Marier [:francois]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #7)
> I don't have any evidence to prove |Compact| causes this issue, it's just a
> guess after analyzing the crash dump.
> I am not sure if its a good idea but since we already fully enable V4 in
> nightly, so V2 will only be used in tracking protection.

Well, we do have a few crash reports in 57 and 58, so we may still be able to see a difference.

> Maybe we can remove "Compact" in Nightly build, it shouldn't affect
> performance too much because most cases are V4.
> 
> Then we can keep monitoring nightly build to see if this may be the root
> cause.

That sounds like a good idea.

Thanks for the detailed investigation Dimi!

Comment 9

[Dimi Lee [:dimi]]

Attachment: Bug 1362761 : Remove Compact from NIGHTLY build for debug purpose.

We suspect nsTArray Compact may cause a crash issue during SafeBrowsing
update. Temporarily remove it from NIGHTLY build, if crash still shows
at NIGHTLY build, we will restore it back.

Review commit: https://reviewboard.mozilla.org/r/188296/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188296/

Comment 10

[Dimi Lee [:dimi]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #7)
> I am not sure if its a good idea but since we already fully enable V4 in
> nightly, so V2 will only be used in tracking protection.
> Maybe we can remove "Compact" in Nightly build, it shouldn't affect
> performance too much because most cases are V4.

Hi Francois,
I was wrong about above comment, those crashes happened in fixed length prefix set which will be used in both V2 and V4.
So if we remove Compact from NIGHTLY build it will affect both V2 and V4.

I am not sure if it is still fine to do it, I will spend some more time to dig deeper.

Comment 11

[François Marier [:francois]]

Comment on attachment 8917283 [details]
Bug 1362761 : Remove Compact from NIGHTLY build for debug purpose.

https://reviewboard.mozilla.org/r/188296/#review193736

Comment 13

[Dimi Lee [:dimi]]

I have spent some time to keep finding other possible root causes, just cannot find one.

Hi francois,
Based on Comment 10, this patch will affect both V2 and V4.
If you think it is still worth doing it, I'll land this patch.

Comment 14

[François Marier [:francois]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #13)
> Based on Comment 10, this patch will affect both V2 and V4.
> If you think it is still worth doing it, I'll land this patch.

I think it would be worth checking whether or not compact() is the cause of the crashes, at least on Nightly.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c70360f53ed9
Remove Compact from NIGHTLY build for debug purpose. r=francois

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 17

[Dimi Lee [:dimi]]

Reopen this bug since patch in Comment 15 is only for debug purpose.

Comment 18

[Dimi Lee [:dimi]]

Sadly this still happens after removing |Compact|[1]
I'll revert the patch landed and see what I can do...

[1] https://crash-stats.mozilla.com/report/index/e5d9df20-0d34-459f-9af5-19d340171028

Comment 19

[Dimi Lee [:dimi]]

Attachment: WIP Patch

This WIP patch implements the checksum mechanism discussed in work week

Comment 20

[Dimi Lee [:dimi]]

Attachment: Bug 1362761 - Add checksum to nsUrlClassifierPrefixSet::mIndexDeltas array

This patch also removes the privious patch in Bug 1362761 that removes
Compact() for nightly build.

Review commit: https://reviewboard.mozilla.org/r/210754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210754/

Comment 21

[François Marier [:francois]]

Comment on attachment 8940563 [details]
Bug 1362761 - Add checksum to nsUrlClassifierPrefixSet::mIndexDeltas array

https://reviewboard.mozilla.org/r/210754/#review221106

::: toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp:515
(Diff revision 1)
> +  // in crash report to find out if this cause the crash.
> +  nsAutoCString checksum;
> +  CalculateTArrayChecksum(mIndexDeltas, checksum);
> +  bool isChecksumMatch = checksum == mIndexDeltasChecksum;
> +  if (!isChecksumMatch) {
> +    LOG(("The content of mIndexDeltas is changed!"));

Perhaps we should make it a full `NS_WARNING()` since it's quite bad if it happens.

Also, I'd suggest "The content of mIndexDeltas is corrupt!" to emphasize the badness of this state :)

::: toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp:588
(Diff revision 1)
> +  crypto = do_CreateInstance(NS_CRYPTO_HASH_CONTRACTID, &rv);
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +
> +  rv = crypto->Init(nsICryptoHash::MD5);

Instead of using MD5, we should probably use CRC32 which will be faster.

There's an implementation in mozilla-central:

https://searchfox.org/mozilla-central/rev/4611b9541894e90a421debb57ddbbcff55c2f369/other-licenses/bsdiff/bsdiff.c#46

that the layout code is already using:

https://searchfox.org/mozilla-central/rev/4611b9541894e90a421debb57ddbbcff55c2f369/layout/style/nsLayoutStylesheetCache.cpp#494

Comment 22

[François Marier [:francois]]

Can be tested by looking for the isChecksumMatched variable in the stack trace of the minidump.

Dimi points out that his patch doesn't fix the problem, rather his intent was to first check whether or not we guessed right about the root cause.

Comment 23

[François Marier [:francois]]

Attachment: Bug 1362761 - Add checksum to nsUrlClassifierPrefixSet::mIndexDeltas array.

Adding a checksum to an array in the URL classifier to test our
theory that the crashes are due to memory corruption.

This patch also restores the Compact() calls that were #ifdef'd
out in bug 1362761 to test a different theory.

Review commit: https://reviewboard.mozilla.org/r/220784/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220784/

Comment 24

[François Marier [:francois]]

Attachment: Bug 1362761 - Improve logging in PrefixSet.

In addition to including the name of the prefix set in all of the
LOG messages, the VariablePrefixSet class now initializes its
dependent fixed-size prefix set correctly.

Review commit: https://reviewboard.mozilla.org/r/220786/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220786/

Comment 25

[François Marier [:francois]]

(In reply to François Marier [:francois] from comment #21)
> ::: toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp:515
> (Diff revision 1)
> > +  // in crash report to find out if this cause the crash.
> > +  nsAutoCString checksum;
> > +  CalculateTArrayChecksum(mIndexDeltas, checksum);
> > +  bool isChecksumMatch = checksum == mIndexDeltasChecksum;
> > +  if (!isChecksumMatch) {
> > +    LOG(("The content of mIndexDeltas is changed!"));
> 
> Perhaps we should make it a full `NS_WARNING()` since it's quite bad if it
> happens.
> 
> Also, I'd suggest "The content of mIndexDeltas is corrupt!" to emphasize the
> badness of this state :)

I will make that change once I've confirmed that this theory was right, at the same time as returning an error instead of just logging the error.

Comment 26

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8951480 [details]
Bug 1362761 - Add checksum to nsUrlClassifierPrefixSet::mIndexDeltas array.

https://reviewboard.mozilla.org/r/220784/#review227852

::: toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp:516
(Diff revision 1)
> +  // in crash report to find out if this cause the crash.
> +  uint32_t checksum;
> +  CalculateTArrayChecksum(mIndexDeltas, &checksum);
> +  bool isChecksumMatch = checksum == mIndexDeltasChecksum;
> +  if (!isChecksumMatch) {
> +    LOG(("The content of mIndexDeltas is changed!"));

If we expect to crash when this happens, what about a MOZ_RELEASE_ASSERT?

Seems handier than having to deal with minidumps?

Or if that's too severe, telemetry? (Maybe that won't catch the affected systems though)

::: toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp:588
(Diff revision 1)
> +
> +  for (uint32_t i = 0; i < aArray.Length(); i++) {
> +    void* hdr = reinterpret_cast<void*&>(aArray[i]);
> +    *outChecksum = ComputeCrc32c(*outChecksum,
> +                                 reinterpret_cast<uint8_t*>(&hdr),
> +                                 sizeof(void*));

Should the sizeof here be sizeof(T)?

The amount of data you are checksumming differs between 32-bit and 64-bit builds, it seems, whereas I doubt T is any different?

Comment 27

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8951481 [details]
Bug 1362761 - Improve logging in PrefixSet.

https://reviewboard.mozilla.org/r/220786/#review227856

Comment 28

[François Marier [:francois]]

Comment on attachment 8951480 [details]
Bug 1362761 - Add checksum to nsUrlClassifierPrefixSet::mIndexDeltas array.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220784/diff/1-2/

Comment 29

[François Marier [:francois]]

Comment on attachment 8951481 [details]
Bug 1362761 - Improve logging in PrefixSet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220786/diff/1-2/

Comment 30

[François Marier [:francois]]

Comment on attachment 8951480 [details]
Bug 1362761 - Add checksum to nsUrlClassifierPrefixSet::mIndexDeltas array.

https://reviewboard.mozilla.org/r/220784/#review227852

> If we expect to crash when this happens, what about a MOZ_RELEASE_ASSERT?
> 
> Seems handier than having to deal with minidumps?
> 
> Or if that's too severe, telemetry? (Maybe that won't catch the affected systems though)

I put in a `MOZ_CRASH()`. You're right, since we suspect memory corruption, it's likely to be a bad day for the browser already. May as well make it easier to detect.

> Should the sizeof here be sizeof(T)?
> 
> The amount of data you are checksumming differs between 32-bit and 64-bit builds, it seems, whereas I doubt T is any different?

I changed the code to make it more obvious what we are doing. We're checksumming the memory address of each array element. Therefore yes, the pointer size will be different on 32-bit v. 64-bit. Does that make more sense or do you see something wrong with this approach?

Comment 31

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8951480 [details]
Bug 1362761 - Add checksum to nsUrlClassifierPrefixSet::mIndexDeltas array.

https://reviewboard.mozilla.org/r/220784/#review229118

Okay, makes sense now realizing that mIndexDeltas is nested array.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a00711bb0e6
Add checksum to nsUrlClassifierPrefixSet::mIndexDeltas array. r=gcp
https://hg.mozilla.org/integration/autoland/rev/923a5ace946a
Improve logging in PrefixSet. r=gcp

Comment 33

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/3a00711bb0e6
https://hg.mozilla.org/mozilla-central/rev/923a5ace946a

Comment 34

[François Marier [:francois]]

Attachment: Bug 1362761 - Make prefix and chunk sets as const as possible. r?dimi

MozReview-Commit-ID: JdnNOxnBAgC

Comment 35

[François Marier [:francois]]

Attachment: Bug 1362761 - Make prefix and chunk sets as const as possible. r?dimi

MozReview-Commit-ID: DhhNOdrlkPF

Comment 36

[François Marier [:francois]]

Attachment: Bug 1362761 - Force file and streams to use smart pointers. r?dimi

MozReview-Commit-ID: GscB9PaaN02

Comment 37

[François Marier [:francois]]

Attachment: Bug 1362761 - Add more specific warnings in case of file corruption. r?dimi

MozReview-Commit-ID: KsgcQWLGulH

Comment 38

[François Marier [:francois]]

Attachment: Bug 1362761 - Safer Clean() and IsEmpty() handling in PrefixSet. r?dimi

This simplifies the logic around clearing the prefix set and also adds
the clearing of the mIndexDeltasChecksum which should have been done
as part of 3a00711bb0e6.

Additionally, the checks for whether or not the prefix set is empty
include some sanity-checking asserts.

Finally, mTotalPrefixes could be out of sync with mIndexPrefixes
and mIndexDeltas if LoadPrefixes() or MakePrefixSet() fail so we
now only update it once all elements have been added successfully.

There is now a release assert to catch grossly out-of-sync (or
corrupt) values of mTotalPrefixes.

MozReview-Commit-ID: BSbyD2dGsUY

Comment 39

[François Marier [:francois]]

Attachment: Bug 1362761 - Make WritePrefixes() more readable. r?dimi

Add assertions to highlight what the various data structures
should look like.

Also assert to ensure that mIndexPrefixes is always the same
length as mIndexDeltas and avoid writing the prefixes to disk
if that's not the case.

Do a single fallible allocation before we create the indexStarts
array instead of checking on each AppendElement() to simplify
the loop and emphasize the number of elements that the array
will receive (indexSize + 1). Remove the last element since we
don't actually end up writing it to disk.

MozReview-Commit-ID: HIg7ZmgaL7x

Comment 40

[François Marier [:francois]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4263ebba6001d44c5c03fe7c4e715fc6d079475c

Comment 41

[François Marier [:francois]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #7)
> Also the |fileSize| returned by |CalculatePreallocateSize|[3] is wrong for
> all crash dumps.
> |CalculatePreallocateSize| function is quite simple it only invovles
> |mTotalPrefixes| & |mIndexPrefixes|.
> 
> Since mTotalPrefixes is just an integer & mIndexPrefixes is a nsTArray, so
> I'll guess mIndexPrefixes is corrupted.
> (Maybe also mIndexDeltas, if everything is corrupted then mCanary.check
> added by thomas should find this issue).

I noticed that we set mTotalPrefixes early:

https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp#115

when we know we could exit this function with an NS_ERROR_OUT_OF_MEMORY.

nsUrlClassifierPrefixSet::SetPrefixes() is the only caller:
https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp#93

and so I added a Clear() when MakePrefixSet() fails.

Also, I think we're supposed to wipe the mIndexDeltasChecksum in Clear() too so I've added that.

I suspect the underlying problem has to do with data structures getting out-of-sync with one another and not getting cleared properly in some out-of-memory conditions.

If this patch series fixes the crashes, we should backout the checksumming patch.

Comment 42

[Phabricator Automation]

Comment on attachment 8991116 [details]
Bug 1362761 - Add more specific warnings in case of file corruption. r?dimi

Dimi Lee[:dimi][:dlee] has approved the revision.

https://phabricator.services.mozilla.com/D2061

Comment 43

[Phabricator Automation]

Comment on attachment 8991119 [details]
Bug 1362761 - Make WritePrefixes() more readable. r?dimi

Dimi Lee[:dimi][:dlee] has approved the revision.

https://phabricator.services.mozilla.com/D2063

Comment 44

[Phabricator Automation]

Comment on attachment 8991117 [details]
Bug 1362761 - Safer Clean() and IsEmpty() handling in PrefixSet. r?dimi

Dimi Lee[:dimi][:dlee] has approved the revision.

https://phabricator.services.mozilla.com/D2062

Comment 45

[Phabricator Automation]

Comment on attachment 8991109 [details]
Bug 1362761 - Make prefix and chunk sets as const as possible. r?dimi

Dimi Lee[:dimi][:dlee] has approved the revision.

https://phabricator.services.mozilla.com/D2058

Comment 46

[Phabricator Automation]

Comment on attachment 8991115 [details]
Bug 1362761 - Force file and streams to use smart pointers. r?dimi

Dimi Lee[:dimi][:dlee] has approved the revision.

https://phabricator.services.mozilla.com/D2060

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13866fb94708
Make prefix and chunk sets as const as possible. r=dimi

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1cb7dfc813e
Force file and streams to use smart pointers. r=dimi

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc89ebc5b5d8
Add more specific warnings in case of file corruption. r=dimi

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6153cafd5002
Safer Clean() and IsEmpty() handling in PrefixSet. r=dimi

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec28801c69d7
Make WritePrefixes() more readable. r=dimi

Comment 52

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/13866fb94708
https://hg.mozilla.org/mozilla-central/rev/e1cb7dfc813e
https://hg.mozilla.org/mozilla-central/rev/cc89ebc5b5d8
https://hg.mozilla.org/mozilla-central/rev/6153cafd5002
https://hg.mozilla.org/mozilla-central/rev/ec28801c69d7

Comment 53

[François Marier [:francois]]

This will hopefully be fixed by the changes that landed in comment 52 (Firefox 63).

If that's the case, we should backout the checksums added in comment 33 and then close this bug.

Comment 54

[Dimi Lee [:dimi]]

just an update that we can still find the crash signature in 63 nightly[1].

For crash reports in 61.0.1, 2.5% crashes happened in MOZ_CRASH("Memory corruption detected in mIndexDeltas.");[2][3]
I suspect this is a sign that this bug is caused by memory corruption even the checksum mechanism only catches a small amount of crashes.

[1] https://crash-stats.mozilla.com/signature/?version=63.0a1&signature=nsUrlClassifierPrefixSet%3A%3AWritePrefixes&date=%3E%3D2018-06-04T21%3A26%3A07.000Z&date=%3C2018-09-04T21%3A26%3A07.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1#aggregations
[2] https://crash-stats.mozilla.com/signature/?moz_crash_reason=Memory%20corruption%20detected%20in%20mIndexDeltas&version=61.0.1&signature=nsUrlClassifierPrefixSet%3A%3AWritePrefixes&date=%3E%3D2018-06-04T17%3A43%3A27.000Z&date=%3C2018-09-04T17%3A43%3A27.000Z#aggregations
[3] https://crash-stats.mozilla.com/signature/?version=61.0.1&signature=nsUrlClassifierPrefixSet%3A%3AWritePrefixes&date=%3E%3D2018-06-04T19%3A43%3A27.000Z&date=%3C2018-09-04T19%3A43%3A27.000Z#aggregations

Comment 55

[François Marier [:francois]]

I looked through the Nightly 63 crashes on Mac: https://crash-stats.mozilla.com/signature/?version=63.0a1&signature=nsUrlClassifierPrefixSet%3A%3AWritePrefixes&date=%3E%3D2018-06-04T21%3A26%3A07.000Z&date=%3C2018-09-04T21%3A26%3A07.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1#reports

and they appear to be on different machines, based on the OS version and the GPU device ID.

I also looked at the memory corruption crashes: https://crash-stats.mozilla.com/signature/?moz_crash_reason=Memory%20corruption%20detected%20in%20mIndexDeltas&version=61.0.1&signature=nsUrlClassifierPrefixSet%3A%3AWritePrefixes&date=%3E%3D2018-06-04T17%3A43%3A27.000Z&date=%3C2018-09-04T17%3A43%3A27.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1#reports

and based on the install time, they also look like different machines.

I noticed one crash while running inside of Virtual Box: https://crash-stats.mozilla.com/report/index/9ae34820-e868-47b6-9a65-9f6e70180830#tab-metadata

though the other ones are using real hard disks / SSDs from many different vendors.

Comment 56

[Marcia Knous [:marcia]]

Adding 64 and 65 as affected per crash stats. For Mac crashes only, this is the #2 browser crash.

Comment 57

[Dimi Lee [:dimi]]

Base on the latest crash reports, I believe this bug is resolved after the fix in Bug 1542744!