Closed Bug 568959 Opened 11 years ago Closed 11 years ago

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

Categories

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

ARM
Android
defect

Tracking

(Not tracked)

RESOLVED FIXED
flash10.2.x-Spicy

People

(Reporter: apiira, Assigned: lhansen)

References

Details

(Whiteboard: has-patch)

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Proposed fix (obsolete) — Splinter Review
releasePCREContext is called from the Android player's PlatformGlobals destructor.
Attachment #448107 - Flags: superreview?(lhansen)
Attachment #448107 - Flags: review?(edwsmith)
Attachment #448107 - Attachment mime type: application/octet-stream → text/plain
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?
Attachment #448107 - Attachment is patch: true
Attachment #448107 - Flags: superreview?(lhansen) → superreview-
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.
Attachment #448107 - Flags: review?(edwsmith)
(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).
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?
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.
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.
Hi Lars.

DISABLE_STATIC_PCRECONTEXT was approved earlier.

https://bugzilla.mozilla.org/show_bug.cgi?id=547935
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.)
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Whiteboard: has-patch
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.
Eh, I probably dropped the ball on pushing it.
Depends on: 450072
Depends on: 547935
No longer depends on: 450072
Priority: -- → P2
Target Milestone: --- → flash10.1.1
Attached patch Cleaned-up fixSplinter Review
Attachment #448107 - Attachment is obsolete: true
tamarin-redux changeset:   4786:4a0ad842e214
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: flash10.1.1 → flash10.2.x-Spicy
You need to log in before you can comment on or make changes to this bug.