568959 - Android fails to call static global object destructor for AvmCore.cpp's PCREContext

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P2)

Description

[Antti Piira]

Android seems to have an OS bug where the destructor of the static global is not called when the library is unloaded. In AvmCore.cpp we have PCREContext that is GCThreadLocal. On construction it will create a system thread local variable key. It should release it in the destructor, but now the destructor doesnt get called and we thus leak the key. 

Currently the Android player loads and unloads the library on every website so we leak it quite often. Eventually (after around 50 websites) the OS starts running out of resources for thread local variables (at least for that thread) and we start failing those. GCHeap relies on those being available for enterFrame and we end up crashing.

Watson bug id #2631345.

Comment 1

[Antti Piira]

Attachment: Proposed fix

releasePCREContext is called from the Android player's PlatformGlobals destructor.

Comment 2

[Lars T Hansen]

Comment on attachment 448107 [details] [diff] [review]
Proposed fix

There's a hint here of an unapproved preprocessor macro DISABLE_STATIC_PCRECONTEXT that is likely unacceptable.  What's going on with that?

Comment 3

[Antti Piira]

I dont know the origin of DISABLE_STATIC_PCRECONTEXT. I dont have the source code with me right now, but believe there was a comment saying that it is to go around Symbian emulator compilation issues. Something to do with the static variable initialization calling to OS APIs.

I saw that the block with DISABLE_STATIC_PCRECONTEXT had empty implementations for the PCREContext related functions. As I added the releasePCREContext function, I also added an empty implementation for it to follow the existing pattern.

Comment 4

[Steven Johnson]

(In reply to comment #3)
> I dont know the origin of DISABLE_STATIC_PCRECONTEXT. I dont have the source
> code with me right now, but believe there was a comment saying that it is to go
> around Symbian emulator compilation issues. Something to do with the static
> variable initialization calling to OS APIs.

IIRC it's a hack that Samuli inserted to deal with the Symbian emulator's limitations; apparently the emulator is, shall we say, suboptimal in a number of ways. That said, I thought it had gone thru the normal check-in channels; if not, let's do so (perhaps converting to the AVMTWEAK format).

Comment 5

[Antti Piira]

So, what should we do about this? I still think that the change to add the releasePCREContext is needed on Android (until the OS bug is fixed). Should we file another bug to clean up the DISABLE_STATIC_PCRECONTEXT preprocessor macro?

Comment 6

[Lars T Hansen]

OK, I've looked at the FP code and your patch and I understand what's going on.

The patch you've offered is fine, modulo the tab characters.  We can land the part that does not contain code for the DISABLE_STATIC_PCRECONTEXT case in tamarin-redux (for 10.2).  If you have an Android branch for 10.1 then you have to manage that patch yourself; we don't have a branch for that in Tamarin.

Samuli will have to fend for himself; if he wants his hack into tamarin-redux it will go through the normal channels and we will handle it then.

I can land your fix in tamarin-redux if you like.  Please let me know.

Comment 7

[Antti Piira]

Sounds good. I already have the fix in Android 10.1. I think it would be be good to land this change to tamarin-redux as well, as I dont know of the time frame the OS bug would be fixed.

I'll ping Samuli and let him know about the DISABLE_STATIC_PRECONTEXT issue.

Thanks.

Comment 8

[Samuli Kekki]

Hi Lars.

DISABLE_STATIC_PCRECONTEXT was approved earlier.

https://bugzilla.mozilla.org/show_bug.cgi?id=547935

Comment 9

[Lars T Hansen]

Aha.  I see that patch never landed.  Even though that should not have to be your responsibility it's probably a good idea to make sure the definitely is marked as "has-patch" in the Whiteboard and definitely assigned to somebody who can land it, so that they will land it.

If you think the patch is complete as it stands then please assign that bug to me and I will clean it up (changing the ad-hoc #ifdef to something visible and tractable like an AVMTWEAK) and push it to tamarin-redux when I take care of Antti's patch.  (I believe tamarin-argo is currently read-only so any other pushing is pretty much up to you.)

Comment 10

[Samuli Kekki]

Hi Lars, yes, the patch in the other bug (547935) still holds. Antti's patch also looks OK. My patch had problems with indentation. Let me know if you need help with Symbian files.

Yes, I didn't follow through the bug, the first time it didn't get applied.

Comment 11

[Steven Johnson]

Eh, I probably dropped the ball on pushing it.

Comment 12

[Lars T Hansen]

Attachment: Cleaned-up fix

Comment 13

[Lars T Hansen]

tamarin-redux changeset:   4786:4a0ad842e214