Web Worker crashes with WX_EXCLUSIVE

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yong Li]

MetaAllocator tries to share one page for assemblies. Currently all threads share one MetaAllocator. When one thread is using a page to execute an assembly, another thread may want to write on it.

Making the allocator thread-specific for WX_EXCLUSIVE is a quick solution.

[Yong Li]

Created attachment 130679 [details]
the patch

[WebKit Review Bot]

Attachment 130679 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/jit/ExecutableAllocator.cpp:107:  g_allocator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/jit/ExecutableAllocator.cpp:122:  g_allocator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 130679 [details]
the patch

Attachment 130679 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11855264

[Gyuyoung Kim]

Comment on attachment 130679 [details]
the patch

Attachment 130679 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11853279

[Build Bot]

Comment on attachment 130679 [details]
the patch

Attachment 130679 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11854270

[Gustavo Noronha (kov)]

Comment on attachment 130679 [details]
the patch

Attachment 130679 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11849377

[Yong Li]

Created attachment 130710 [details]
the correct patch

The previous patch was actually an obsolete one that never worked

[Yong Li]

Darn... Some implementations of TCMalloc_SpinLock has Finalize, and some don't...

Can we just use Mutex defined in ThreadingPrimitives.h?

[Early Warning System Bot]

Comment on attachment 130710 [details]
the correct patch

Attachment 130710 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11850444

[Filip Pizlo]

Comment on attachment 130710 [details]
the correct patch

This would break JavaScriptCore.

A JSC VM instance may be used from multiple threads.

[Yong Li]

(In reply to comment #10)
> (From update of attachment 130710 [details])
> This would break JavaScriptCore.
> 
> A JSC VM instance may be used from multiple threads.

Then any suggestion about how to recover the ASSEMBLER_WX_EXCLUSIVE?

Can I wrap the code with something like #if ENABLE(ASSEMBLER_WX_EXCLUSIVE) && USE(JSC_SINGLE_THREADED)?

[Gavin Barraclough]

Phil,

I guess fixing this would mean splitting the ExecutableAllocator to be tiered into a have a locked, global page allocation, and a per-global-data sub page allocation pool.  What do you think?

G.

[Filip Pizlo]

(In reply to comment #12)
> Phil,
> 
> I guess fixing this would mean splitting the ExecutableAllocator to be tiered into a have a locked, global page allocation, and a per-global-data sub page allocation pool.  What do you think?
> 
> G.

I tend to think that WX_EXCLUSIVE should just be thread safe.  Is there any reason why it cannot be?

The MetaAllocator has a simple thread safety contract: it protects itself with a lock.  Hence once its code runs, we know that we're serialized.

If WX_EXCLUSIVE needs to do additional things, it should also protect itself with a lock - either the MetaAllocator's lock or a different one.  Probably doesn't matter.

I don't buy that we should be splitting the executable memory allocator into separate memory allocators.  This just increases fragmentation, which is what we are trying to avoid at all costs.

[Filip Pizlo]

(In reply to comment #0)
> MetaAllocator tries to share one page for assemblies. Currently all threads share one MetaAllocator. When one thread is using a page to execute an assembly, another thread may want to write on it.
> 
> Making the allocator thread-specific for WX_EXCLUSIVE is a quick solution.

I think for this code I would prefer that we do the right solution, rather than the quick one, though see my comment below.

[Filip Pizlo]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 130710 [details] [details])
> > This would break JavaScriptCore.
> > 
> > A JSC VM instance may be used from multiple threads.
> 
> Then any suggestion about how to recover the ASSEMBLER_WX_EXCLUSIVE?
> 
> Can I wrap the code with something like #if ENABLE(ASSEMBLER_WX_EXCLUSIVE) && USE(JSC_SINGLE_THREADED)?

I would be OK with this, but please open a bug about making WX_EXCLUSIVE thread safe.

[Yong Li]

(In reply to comment #15)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (From update of attachment 130710 [details] [details] [details])
> > > This would break JavaScriptCore.
> > > 
> > > A JSC VM instance may be used from multiple threads.
> > 
> > Then any suggestion about how to recover the ASSEMBLER_WX_EXCLUSIVE?
> > 
> > Can I wrap the code with something like #if ENABLE(ASSEMBLER_WX_EXCLUSIVE) && USE(JSC_SINGLE_THREADED)?
> 
> I would be OK with this, but please open a bug about making WX_EXCLUSIVE thread safe.

Was WX_EXCLUSIVE ever thread-safe?

1) If not, then wrapping with another #define seems not necessary.
2) If yes, I'm thinking we can make every global object own a MetaAllocator. So the global object is still thread-safe, and there is no conflict from web worker. Do you think this is right thing to do?

[Yong Li]

Created attachment 131071 [details]
For WX_EXCLUSIVE, let each global object own a meta allocator

[Yong Li]

Created attachment 131074 [details]
One MetaAllocator per JS global object (only for WX_EXCLUSIVE)

Had to add a dtor and put it to the cpp file otherwise OwnPtr doesn't know how to delete DemandExecutableAllocator.

[Filip Pizlo]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #11)
> > > (In reply to comment #10)
> > > > (From update of attachment 130710 [details] [details] [details] [details])
> > > > This would break JavaScriptCore.
> > > > 
> > > > A JSC VM instance may be used from multiple threads.
> > > 
> > > Then any suggestion about how to recover the ASSEMBLER_WX_EXCLUSIVE?
> > > 
> > > Can I wrap the code with something like #if ENABLE(ASSEMBLER_WX_EXCLUSIVE) && USE(JSC_SINGLE_THREADED)?
> > 
> > I would be OK with this, but please open a bug about making WX_EXCLUSIVE thread safe.
> 
> Was WX_EXCLUSIVE ever thread-safe?

Yes, because the old allocator would put each piece of code onto a separate page.

> 
> 1) If not, then wrapping with another #define seems not necessary.
> 2) If yes, I'm thinking we can make every global object own a MetaAllocator. So the global object is still thread-safe, and there is no conflict from web worker. Do you think this is right thing to do?

I don't understand why you think this is an easier solution than putting a lock around methods that do the work of WX_EXCLUSIVE.  That seems so much easier and will result in a smaller patch.  Can you explain why you did not do it that way?

[Yong Li]

Created attachment 131080 [details]
fix build error for ExecutableAllocatorFixedVMPool

[Gavin Barraclough]

(In reply to comment #19)
> I don't understand why you think this is an easier solution than putting a lock around methods that do the work of WX_EXCLUSIVE.  That seems so much easier and will result in a smaller patch.  Can you explain why you did not do it that way?

Is this the whole problem?  I was thinking we had a scenario where:

Thread A is allocated space on page X.
Thread A makes page X writable.
Thread A generates code on page X.
Thread A makes page X executable.
Thread A starts running code on page X.
Thread B is allocated space on page X.
Thread B makes page X writable.
Thread A crashes.

Just locking the region in with the writable/executable permissions are changed doesn't seem to be enough, you'd need to interrupt other running threads, wouldn't you?

[Filip Pizlo]

(In reply to comment #21)
> (In reply to comment #19)
> > I don't understand why you think this is an easier solution than putting a lock around methods that do the work of WX_EXCLUSIVE.  That seems so much easier and will result in a smaller patch.  Can you explain why you did not do it that way?
> 
> Is this the whole problem?  I was thinking we had a scenario where:
> 
> Thread A is allocated space on page X.
> Thread A makes page X writable.
> Thread A generates code on page X.
> Thread A makes page X executable.
> Thread A starts running code on page X.
> Thread B is allocated space on page X.
> Thread B makes page X writable.
> Thread A crashes.
> 
> Just locking the region in with the writable/executable permissions are changed doesn't seem to be enough, you'd need to interrupt other running threads, wouldn't you?

Ahh!  Good point.  I forgot that WX_EXCLUSIVE meant flipping pages from exec-only to write-only.

[WebKit Review Bot]

Comment on attachment 131080 [details]
fix build error for ExecutableAllocatorFixedVMPool

Clearing flags on attachment: 131080

Committed r110379: <http://trac.webkit.org/changeset/110379>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.