547935 - Initialization of PCREContext leads to crash on Symbian emulator without EPOCALLOWDLLDATA

Status: RESOLVED FIXED

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

Description

[Kenichi Ando]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9.2) Gecko/20100115 Firefox/3.6 GTBDFff GTB7.0 (.NET CLR 3.5.30729)
Build Identifier: 

There is the following static variable in the global scope in AVMCore.cpp.

	static GCThreadLocal<Toplevel*> PCREContext;

This PCREContext is treated as a Writable Static Data(WSD) on Symbian OS. Also Symbian emulator has the following restriction on WSD.

"The only restriction is that if the data’s initialisers call any Symbian OS kernel functions (i.e. executive calls) the emulator will fault."

Reference:
http://developer.symbian.org/wiki/index.php/Symbian_Platform_Support_for_Writeable_Static_Data_in_DLLs

Since AVM calls pthread_key_create, which is a kernel function, from the static_initializer, the emulator crashes because of the above restriction.



Reproducible: Always

Steps to Reproduce:
1. Launch VM.

2. PCREContext is created in the initialization process, because it's a global static variable.

3. The constructor of MMgc::GCThreadLocal is invoked, then it calls VMPI_tlsCreate and pthread_key_create.
Actual Results:  
Crashes due to "FAULT: BAD DLL STATIC 0x00000000 (0)"

Backtrace:
27 VMPI_tlsCreate() ...\VMPI\ThreadsPosix.cpp:45 0x47c9820d	
26 MMgc::GCThreadLocal<avmplus::Toplevel *>::GCThreadLocal() ...\MMgc\GCThreadLocal.h:52 0x47deb537	
25 $static_initializer$415() ...\MMgc\WriteBarrier.h:279 0x47deb49d	
24 invokeTable() Z:\src\cedar\generic\base\e32\include\win32crt.h:119 0x47c976c3	
23 _E32Dll() Z:\src\cedar\generic\base\e32\euser\epoc\win32\uc_dll.cpp:60 0x47c97964	



Expected Results:  
No crash

Made a patch for this bug. Changed GCThreadLocal.h. 
Applying "lazy initialization" pattern to MMgc::GCThreadLocal solves this problem, because the constructor of GCThreadLocal does not call a kernel function(pthread_key_create) in the initialization process.

Comment 1

[Kenichi Ando]

Attachment: proposed patch - GCThreadLocal.h

Comment 2

[Kenichi Ando]

Attachment: proposed patch - GCThreadLocal.h diff

Comment 3

[Lars T Hansen]

Comment on attachment 428396 [details] [diff] [review]
proposed patch - GCThreadLocal.h diff

Patch rejected because:

- the patch introduces a race condition in the initialization of that global variable, the purpose of having them static globals is for the initialization to be executed on the main thread always.

- returning NULL when the thread local is not valid is wrong for several reasons, notably because the T is not always a type that has NULL in its value set and because the consequence of returning NULL for the -> operator is an immediate crash.

- the patch is incorrect, it shows the new code as being removed rather than added.

Also, I don't know if we're inclined to work around bugs in the Symbian emulator, but we'll have to discuss that.

Comment 4

[Samuli Kekki]

This patch is to work around an issue, that only one process (a time) can load Flash dll successfully in the Symbian emulator. Multiple processes can load Flash dll in device without problems.

An example: the first process can be a browser, the second some other user application, like desktop widget. Flash/FlashLite can be used in Symbian desktop widgets.

Comment 5

[Kenichi Ando]

Hi Lars, thanks for the review.

As Samuli mentioned the above, some customer who develops a product on Symbian ran into this issue. Since this is a serious problem for them, they requested us to fix this issue. They also claims that the AVM worked on the Symbian emulator before introducing PCREContext variable.

What Symbian emulator can't do is to call kernel functions from the initialization of writable global (static) variables.
That's why I moved the code from the constructor to the other functions. But now that I understand this patch has some problems, we have to find other work around.

Comment 6

[Steven Johnson]

Is there a way to determine (either compiletime or runtime) that we are targeting the emulator?

Comment 7

[Samuli Kekki]

Sure, in the current emulator you can check for __WINSCW__ at compile time. Not sure about runtime check.

Comment 8

[Samuli Kekki]

These would fix the problem: make PCREContext constructor not call any VMPI function or make PCREContext a non-static variable. Not sure it the vm team can consider these.

Comment 9

[Steven Johnson]

(In reply to comment #8)
> These would fix the problem: make PCREContext constructor not call any VMPI
> function or make PCREContext a non-static variable. Not sure it the vm team can
> consider these.

What if we made PCREContext non-static, but only ifdef __WINSCW__? Then all other existing targets would be unaffected.

Comment 10

[Lars T Hansen]

(In reply to comment #9)
> (In reply to comment #8)
> > These would fix the problem: make PCREContext constructor not call any VMPI
> > function or make PCREContext a non-static variable. Not sure it the vm team can
> > consider these.
> 
> What if we made PCREContext non-static, but only ifdef __WINSCW__? Then all
> other existing targets would be unaffected.

Only if you're sure that it'll be created by the main thread before any other threads get created, or you have a race condition on the creation (which is the main reason why it was static to begin with).

Comment 11

[Samuli Kekki]

Hi Lars, could you tell more about the race condition and why it is important that the main thread creates PCREContext. What are the other threads you refer to? Isn't avm single threaded?

Comment 12

[Samuli Kekki]

= single threaded in the context of Flash Player

Comment 13

[Samuli Kekki]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > These would fix the problem: make PCREContext constructor not call any VMPI
> > > function or make PCREContext a non-static variable. Not sure it the vm team can
> > > consider these.
> > 
> > What if we made PCREContext non-static, but only ifdef __WINSCW__? Then all
> > other existing targets would be unaffected.
> Only if you're sure that it'll be created by the main thread before any other
> threads get created, or you have a race condition on the creation (which is the
> main reason why it was static to begin with).

Comment 14

[Samuli Kekki]

Comment on attachment 428396 [details] [diff] [review]
proposed patch - GCThreadLocal.h diff

A new patch coming

Comment 15

[Samuli Kekki]

Attachment: Disable PCRE stack check for Symbian emulator build to fix the problem

This change was approved by Steven for FP earlier. Please take a look if it's good enough for Tamarin as is or if you need any changes.

Comment 16

[Samuli Kekki]

I can see some } difficulties that I don't see in my code editor. Do you want a net patch or could you fix this locally before submitting?

Comment 17

[Samuli Kekki]

" a new patch "(In reply to comment #16)
> net patch or could you fix this locally before submitting?

Comment 18

[Lars T Hansen]

Attachment: Restructured patch

Same patch, but (a) tabs vs spaces fixed, (b) ad-hoc define refactored as AVMTWEAK_EPOC_EMULATOR, (c) the enabling define moved from symbian-platform.h to system-selection.h, where it makes the most sense IMO.

Comment 19

[Lars T Hansen]

Samuli, any input from you is appreciated as well.

Comment 20

[Lars T Hansen]

Pushed in anticipation of review:
tamarin-redux changeset:   4785:e109b4ac2b77