Bug 135143

Summary: JSLock release should only modify the AtomicStringTable if it modified in acquire
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, commit-queue, ggaren, joepeck, mhahnenberg, thorton
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 135192    
Bug Blocks:    
Attachments:
Description Flags
[PATCH] Proposed Fix
psolanki: review+
[PATCH] Proposed Fix
darin: review+
[PATCH] For Landing none

Description Joseph Pecoraro 2014-07-21 19:30:04 PDT
JSLock could have an imbalanced and unexpected change to the global thread data's AtomicStringTable.

The expected balance between JSLock::didAcquireLock and JSLock::willReleaseLock wouldn't happen if the VM was not set (during destruction of a VM).
Comment 1 Joseph Pecoraro 2014-07-21 19:34:48 PDT
Created attachment 235264 [details]
[PATCH] Proposed Fix

I have been trying to create a test for this but it is proving difficult.

My plan is to create a JSContext on a non-main thread (so a non-main AtomicStringTable), delete the JSContext on the main thread (such that JSLock would have unbalanced lock and release leaving the wrong AtomicStringTable). But it seems I'm missing some complexity. In any case, the reproducible case I have (a larger application) reproduced the issue reliably.
Comment 2 Joseph Pecoraro 2014-07-21 19:48:23 PDT
<rdar://problem/17041912>
Comment 3 Pratik Solanki 2014-07-22 10:25:14 PDT
Comment on attachment 235264 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=235264&action=review

Patch looks fine to me but would be good if Geoff took a look as well.

> Source/JavaScriptCore/runtime/JSLock.cpp:170
> +    if (m_vm) {

Early return?
Comment 4 Joseph Pecoraro 2014-07-22 15:10:41 PDT
r171367: <http://trac.webkit.org/changeset/171367>
Comment 5 WebKit Commit Bot 2014-07-23 03:28:57 PDT
Re-opened since this is blocked by bug 135192
Comment 6 Tim Horton 2014-07-23 03:29:23 PDT
Stealing ap's message from the wrong bug:

"This change broke three API tests, so I'm going to roll out.

Tests that failed:
  WKUserContentController.ScriptMessageHandlerWithNavigation
  WebKit2.RestoreSessionStateContainingFormData
Tests that timed out:
  DeviceScaleFactorOnBack.WebKit2"

The UI process is stuck in a HashTable lock adding a string to the AtomicString table from inside decidePolicyForResponse
Comment 7 Joseph Pecoraro 2014-07-23 11:08:11 PDT
(In reply to comment #6)
> Stealing ap's message from the wrong bug:
> 
> "This change broke three API tests, so I'm going to roll out.
> 
> Tests that failed:
>   WKUserContentController.ScriptMessageHandlerWithNavigation
>   WebKit2.RestoreSessionStateContainingFormData
> Tests that timed out:
>   DeviceScaleFactorOnBack.WebKit2"

I can reproduce.

Thinking about things, now we might incorrectly balance in the opposite way after this change.

We need to handle what will happen if:
  - we acquire the lock with a vm (set the atomic string table)
  - m_vm gets cleared
  - we release the lock without a vm
    => we need to restore the atomic string table.

Making the appropriate changes the tests all pass. I need to test the original case and then I'll put up another patch.
Comment 8 Joseph Pecoraro 2014-07-23 14:15:36 PDT
Created attachment 235376 [details]
[PATCH] Proposed Fix
Comment 9 Geoffrey Garen 2014-07-23 14:20:25 PDT
Comment on attachment 235376 [details]
[PATCH] Proposed Fix

What if we set the table on entry, and it was null on entry? How will it get back to null?
Comment 10 Joseph Pecoraro 2014-07-23 15:56:01 PDT
(In reply to comment #9)
> (From update of attachment 235376 [details])
> What if we set the table on entry, and it was null on entry? How will it get back to null?

Good point. My understanding is that the WTFThreadData's AtomicStringTable can (and should) never be null. I could ASSERT that now, and perhaps ultimately we could move to references. Alternatively we could have a bool alongside the pointer.
Comment 11 Geoffrey Garen 2014-07-24 10:06:53 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 235376 [details] [details])
> > What if we set the table on entry, and it was null on entry? How will it get back to null?
> 
> Good point. My understanding is that the WTFThreadData's AtomicStringTable can (and should) never be null. I could ASSERT that now, and perhaps ultimately we could move to references. Alternatively we could have a bool alongside the pointer.

It looks like WTFThreadData always sets a default table, so I think you're right. Let's ASSERT.
Comment 12 Joseph Pecoraro 2014-07-24 16:55:54 PDT
Created attachment 235474 [details]
[PATCH] For Landing

Added ASSERTs. Re-ran my tests without a hitch.
Comment 13 Joseph Pecoraro 2014-07-24 16:57:10 PDT
r171558: <http://trac.webkit.org/changeset/171558>