Bug 28904 – [V8] Another pass at releasing the storage mutex.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

Another pass at releasing the storage mutex (when applicable) when we finish executing JavaScript.  http://dev.w3.org/html5/spec/Overview.html#storage-mutex describes the required behavior.

As it turns out, https://bugs.webkit.org/show_bug.cgi?id=28789 didn't solve the problem correctly.  First of all, I missed that events and timeouts go through a different call path.  Second of all, I didn't consider recursion correctly.  Third of all, my check to see if LocalStorage is alive actually instantiates it if it isn't yet.  Fourth, I forgot DOM_STORAGE guards around it.  So I'm reverting that change completely and doing it more cleanly.  Unfortunately, the solution isn't as portable (and thus I've left out the JSC implementation for now).

       * bindings/js/ScriptController.cpp:
        (WebCore::ScriptController::evaluate): revert
        * bindings/v8/ScriptController.cpp:
        (WebCore::ScriptController::evaluate): revert
        * bindings/v8/V8Proxy.cpp:
        (WebCore::V8Proxy::runScript): Add releaseStorageMutex call.  A small amount of clean up.
        (WebCore::V8Proxy::callFunction):  Add releaseStorageMutex call.  A small amount of clean up.
        (WebCore::V8Proxy::releaseStorageMutex):  Check to see if localStorage has been instantiated.  If so, tell it to unlock all storage areas.
        * bindings/v8/V8Proxy.h:
        * page/PageGroup.h:
        (WebCore::PageGroup::hasLocalStorage):  Make hasLocalStorage public.

[Jeremy Orlow]

Marking as V8 since this will need a V8 reviewer (even though it's not completely V8 specific...but the non-v8 parts are trivial).

[Jeremy Orlow]

Created attachment 38914 [details]
Patch v1

[Eric Seidel (no email)]

Comment on attachment 38914 [details]
Patch v1

Can't we test this?  It seems so, in which case this really needs tests.  r- for lack of testing (or explanation why it's impossible).

[Jeremy Orlow]

Created attachment 38916 [details]
Patch v1

[Jeremy Orlow]

Made the fixes Eric asked for.  He told me he'd feel more comfortable if dglazkov or abarth did the actual sign off, though.  (Note that Mads and Mbelshe looked at an earlier version of this change as well.)

[Adam Barth]

Comment on attachment 38916 [details]
Patch v1

This looks fine to me.  I'm not the expert on this topic, but if Mads and Mike signed off too, then we're probably ok.

[Eric Seidel (no email)]

Comment on attachment 38916 [details]
Patch v1

Rejecting patch 38916 from commit-queue.  This patch will require manual commit.

['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

[Adam Barth]

Comment on attachment 38916 [details]
Patch v1

Clearing flags on attachment: 38916

Committed r47973: <http://trac.webkit.org/changeset/47973>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.