Bug 135143 – JSLock release should only modify the AtomicStringTable if it modified in acquire

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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).

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

<rdar://problem/17041912>

[Pratik Solanki]

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?

[Joseph Pecoraro]

r171367: <http://trac.webkit.org/changeset/171367>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 135192

[Tim Horton]

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

[Joseph Pecoraro]

(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.

[Joseph Pecoraro]

Created attachment 235376 [details]
[PATCH] Proposed Fix

[Geoffrey Garen]

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?

[Joseph Pecoraro]

(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.

[Geoffrey Garen]

(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.

[Joseph Pecoraro]

Created attachment 235474 [details]
[PATCH] For Landing

Added ASSERTs. Re-ran my tests without a hitch.

[Joseph Pecoraro]

r171558: <http://trac.webkit.org/changeset/171558>