1107775 - Make deferred finalization more re-entrant safe

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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...

Comment 1

[Andrew McCreight [:mccr8]]

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.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

(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).

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Oh, MOZ_CRASH is also in release builds, so I don't want that.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

https://hg.mozilla.org/integration/mozilla-inbound/rev/81bec89d093b

Comment 5

[Carsten Book [:Tomcat]]

sorry backed out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4384744&repo=mozilla-inbound

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Oh, right, stupid me.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 2->3 -w interdiff

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e04c5b88fcd

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7e04c5b88fcd

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

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

Comment 14

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

The patch did fix some possibly theoretical issues, but apparently not the one we want to fix.