Make deferred finalization more re-entrant safe

RESOLVED FIXED in mozilla37

Status

()

Core
XPCOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

36 Branch
mozilla37
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Created attachment 8532324 [details] [diff] [review]
reentrant_safe_finalization.diff

If we re-enter ReleaseNow, mFinalizeRunnable will be non-null after ReleaseNow(false), so
we just need to continue doing what we were doing.

Since this should be rather rare case, I decided to not reschedule new FinalizeRunnable
after mFinalizeRunnable is done. We could just rely on the next gc to reschedule such.

https://tbpl.mozilla.org/?tree=Try&rev=10e1ea2623aa

crossing fingers, throwing salt over shoulders...
Attachment #8532324 - Flags: review?(continuation)
Comment on attachment 8532324 [details] [diff] [review]
reentrant_safe_finalization.diff

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

Thanks for fixing this.  Hopefully it solves the crashes...

::: xpcom/base/CycleCollectedJSRuntime.cpp
@@ +60,1 @@
>  #include "mozilla/ArrayUtils.h"

nit: Auto is after Array in the alphabet

@@ +1145,5 @@
>  
>  void
>  IncrementalFinalizeRunnable::ReleaseNow(bool aLimited)
>  {
> +  if (mReleasing) {

Shouldn't this assert if it happens?  Not that we're likely to ever see it, but it would be nice to know if it happens, and document that this is bad.
Attachment #8532324 - Flags: review?(continuation) → review+
(In reply to Andrew McCreight [:mccr8] from comment #1)
> >  #include "mozilla/ArrayUtils.h"
> 
> nit: Auto is after Array in the alphabet
All sorts of skills one needs to have, like know the alphabet :)

> >  IncrementalFinalizeRunnable::ReleaseNow(bool aLimited)
> >  {
> > +  if (mReleasing) {
> 
> Shouldn't this assert if it happens?  Not that we're likely to ever see it,
> but it would be nice to know if it happens, and document that this is bad.

I guess I could add a MOZ_ASSERT or MOZ_CRASH here (debug only).
Oh, MOZ_CRASH is also in release builds, so I don't want that.
Oh, right, stupid me.
https://hg.mozilla.org/mozilla-central/rev/7e04c5b88fcd
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
justdave, if you still see bug 997908 relatively often, could you perhaps try the next Nightly to see if this bug has helped with it.
Flags: needinfo?(justdave)
This patch has *not* fixed the ReleaseSliceNow() crashes:

bp-25788c84-9d2b-4a6e-b7b2-b96872141208

But it's still possible that it's reduced their frequency.  I'll look again in another week.
Flags: needinfo?(justdave)
I just looked again for crash stats on all platforms (not just OS X, as I did before).  This patch might actually have *increased* the frequency of the ReleaseSliceNow crashes on Windows:

https://crash-stats.mozilla.com/report/list?signature=ReleaseSliceNow%28unsigned+int%2C+void*%29&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&version=Firefox%3A37.0a1&hang_type=any&date=2014-12-10+17%3A00%3A00&range_value=1#tab-reports
Or maybe some other patch increased the frequency on Windows, and this patch simply had no effect.  This patch (presumably) first appeared in the 2014-12-06 m-c nightly, but the increase on Windows seems to start with the 2014-12-08 m-c nightly.

Let's keep an eye on the stats for both Windows and OS X.
The patch did fix some possibly theoretical issues, but apparently not the one we want to fix.
You need to log in before you can comment on or make changes to this bug.