1123840 - crash in libsystem_platform.dylib@0x51e0

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect)

Description

[Benoit Girard (:BenWa)]

This bug was filed from the Socorro interface and is 
report bp-bc11fe31-0e93-4f6d-9322-b69c62150120.
=============================================================

Reproducible for me on page:
http://www.newscientist.com/article/dn26813-epic-cosmic-radio-burst-finally-seen-in-real-time.html

I saw someone run into this. I think it was :gps

Comment 1

[Benoit Girard (:BenWa)]

[Tracking Requested - why for this release]:
Reproducible crash

Comment 2

[Benoit Girard (:BenWa)]

:mayhemer can you triage this to the right person? It's reproducible so it should be easy.

Comment 3

[Gregory Szorc [:gps]]

I worked around it by clearing out my cache. I suspect the "read from cache" code path is buggy. My crashes were https://crash-stats.mozilla.com/report/index/e057151c-4485-4f72-b13c-d62142150119 and https://crash-stats.mozilla.com/report/index/afd7d0b7-4483-4fc4-a503-196122150119

Comment 4

[Benoit Girard (:BenWa)]

Ok, I'll leave things on my cache so that I can test builds.

Comment 5

[Honza Bambas (:mayhemer)]

This is my code.  Taking now.

Comment 6

[Honza Bambas (:mayhemer)]

Benoit, do you have flash installed?  Any other things I should try to change/install to reproduce?  Simply going to the page, restart, navigating again doesn't crash for me (Nightly debug).

Comment 7

[Benoit Girard (:BenWa)]

I have flash 16.0.0.257 & Adblock installed. Just hitting the pages crashes for me.

Comment 8

[Benoit Girard (:BenWa)]

Ok well it seems the crash is no longer reproducible for me.

Comment 9

[Honza Bambas (:mayhemer)]

gps, any way to reproduce?

anybody, if you are able to repro, please run with logging:
NSPR_LOG_MODULES=timestamp,nsHttp:4,cache2:5

Thanks.

Comment 10

[Gregory Szorc [:gps]]

I blew away my cache. But it appears BenWa hasn't.

Comment 11

[Honza Bambas (:mayhemer)]

So, happened to me few times.  Logging changes timing so, could not get a log.  I decided to attach a debugger, but after download of sources (me, one idiot!) I cannot reproduce any longer.  Will occasionally re-try.

Comment 12

[Honza Bambas (:mayhemer)]

And I think I have a theory.  There is reinsert of a callback with intention to put it to the same place as it was before (InsertElementAt(i)).  But when the array were altered because of re-entrancy, we may have fewer elements in the array and the i will point past the end+1.  I may simply add a check and insert at max(end,i).  It's not at all critical to put the callback at the same place.

Comment 13

[Benoit Girard (:BenWa)]

I'll run the log if I can reproduce it again.

Comment 16

[Honza Bambas (:mayhemer)]

Attachment: v1

For explanation see comment 12.

I unfortunately don't have STR to check this is the fix.  I can crash on the two reported URLs, but not when I want to check...  Murphy's law...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d187a373858

Comment 17

[Syd Polk :sydpolk]

I seem to have reproduced this by going to my Google Drive page and right clicking on a file. The URL:

https://drive.google.com/a/mozilla.com/#my-drive (of course, I was logged in as me)

The crash report:

https://crash-stats.mozilla.com/report/index/7a0f21fe-2b21-4f50-8712-18ad12150121

Comment 18

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #12)
> And I think I have a theory.  There is reinsert of a callback with intention
> to put it to the same place as it was before (InsertElementAt(i)).  But when
> the array were altered because of re-entrancy, we may have fewer elements in
> the array and the i will point past the end+1.

Is it OK when we skip some callback or process some callback twice in the while loop in InvokeCallbacks()? This will happen when the array is changed while InvokeCallback() is called.

Comment 19

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #18)
> (In reply to Honza Bambas (:mayhemer) from comment #12)
> > And I think I have a theory.  There is reinsert of a callback with intention
> > to put it to the same place as it was before (InsertElementAt(i)).  But when
> > the array were altered because of re-entrancy, we may have fewer elements in
> > the array and the i will point past the end+1.
> 
> Is it OK when we skip some callback or process some callback twice in the
> while loop in InvokeCallbacks()? This will happen when the array is changed
> while InvokeCallback() is called.

I first remove a callback (under the lock), then process it, when it needs to be called again, I readd it back (now faulty code) to be re-invoked on the next callbacks invocation loop.

InvokeCallback() opens the lock and other thread (or the same via reentry) may modify the array (remove from it).  We cannot skip a callback since I'm trying to put the reverted callback at the place it was.  That will not leave any earlier added callback behind.  We cannot call a callback more then once during the same loop, we also cannot skip during the same loop (as explained above).  It's OK to check on the callback on next call, we do it all the time as I can see in the logs, and it's OK, since the callback will not invoke until some preconditins are met (I think the most obvious case is the "wait for complete entry" state, which should be cached, if performance is considered.)

Does that answer the question?

Comment 20

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Thanks.  Fixed osx/lnx build problem.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/098d965662be

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/098d965662be

Comment 23

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Socorro [1] shows zero crashes with this exact signature over the past 4 weeks, and, since no one reports this still happening, I'm closing it.

[1] - https://crash-stats.mozilla.com/report/list?range_unit=days&range_value=28&signature=libsystem_platform.dylib%400x51e0#tab-sigsummary