Bug 91251 - [V8] String wrappers should be Independent
Summary: [V8] String wrappers should be Independent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on: 91317
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-13 08:41 PDT by Kentaro Hara
Modified: 2012-07-24 11:43 PDT (History)
8 users (show)

See Also:


Attachments
Patch (1.63 KB, patch)
2012-07-13 08:43 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (3.41 KB, patch)
2012-07-24 00:13 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (3.63 KB, patch)
2012-07-24 00:29 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-07-13 08:41:10 PDT
Currently V8 String wrappers are not marked Independent. By marking them Independent, they can be reclaimed by the scavenger GC.
Comment 1 Kentaro Hara 2012-07-13 08:43:39 PDT
Created attachment 152269 [details]
Patch
Comment 2 Adam Barth 2012-07-13 10:20:41 PDT
Comment on attachment 152269 [details]
Patch

Ok.
Comment 3 WebKit Review Bot 2012-07-13 12:05:34 PDT
Comment on attachment 152269 [details]
Patch

Clearing flags on attachment: 152269

Committed r122614: <http://trac.webkit.org/changeset/122614>
Comment 4 WebKit Review Bot 2012-07-13 12:05:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Adam Barth 2012-07-13 23:43:17 PDT
Shall we revert the change and study the crashes?
Comment 7 Ryosuke Niwa 2012-07-13 23:45:37 PDT
(In reply to comment #6)
> Shall we revert the change and study the crashes?

That makes sense given that the blame list doesn't have other suspicious changes. Meanwhile, I'll try to confirm that this is the culprit by locally building & running tests.
Comment 8 Ryosuke Niwa 2012-07-14 01:51:35 PDT
Confirmed.
Comment 9 WebKit Review Bot 2012-07-14 01:52:37 PDT
Re-opened since this is blocked by 91317
Comment 10 Kentaro Hara 2012-07-16 00:56:42 PDT
arv: Do you know why this patch is wrong? My question is whether making String wrappers independent is a wrong fix, or making String wrappers independent is a correct fix but some V8 binding code is wrong.

For example, worker-location.html fails with the following diff:

--- /usr/local/google/home/haraken/chrome-build/src/webkit/Release/layout-test-results/fast/workers/worker-location-expected.txt 
+++ /usr/local/google/home/haraken/chrome-build/src/webkit/Release/layout-test-results/fast/workers/worker-location-actual.txt 
@@ -3,7 +3,7 @@
 WorkerLocation: function WorkerLocation() { [native code] }
 typeof location: object
 location: file:<...>/fast/workers/resources/worker-common.js
-location.href: file:<...>/fast/workers/resources/worker-common.js
+location.href: [object MessageEvent]
 location.protocol: file:
 location.host: 
 location.hostname:
Comment 11 Erik Arvidsson 2012-07-16 15:18:40 PDT
(In reply to comment #10)
> arv: Do you know why this patch is wrong? My question is whether making String wrappers independent is a wrong fix, or making String wrappers independent is a correct fix but some V8 binding code is wrong.

I don't know enough about this...

Looking more at the code it seems like the callback for the weak handle might not be able to get the correct isolate

static void cachedStringCallback(v8::Persistent<v8::Value> wrapper, void* parameter)
{
    StringImpl* stringImpl = static_cast<StringImpl*>(parameter);
    V8BindingPerIsolateData::current()->stringCache()->remove(stringImpl);
    wrapper.Dispose();
    stringImpl->deref();
}

> For example, worker-location.html fails with the following diff:
> -location.href: file:<...>/fast/workers/resources/worker-common.js
> +location.href: [object MessageEvent]

This is strange. It seems like there was an error invoking the right toString method here.
Comment 12 Kentaro Hara 2012-07-16 23:25:21 PDT
> Looking more at the code it seems like the callback for the weak handle might not be able to get the correct isolate

Good point! Thanks arv.

Given that no one has implicit references to String wrappers, I think this patch should work fine. I'll look into it in detail.
Comment 13 Kentaro Hara 2012-07-24 00:13:50 PDT
Created attachment 153973 [details]
Patch
Comment 14 Dan Carney 2012-07-24 00:25:08 PDT
(In reply to comment #13)
> Created an attachment (id=153973) [details]
> Patch

Great!  I wonder if the following would even be better:

     if( stringImpl == m_lastStringImpl )
        m_lastStringImpl = 0;

Then the last string cache will continue to work after an arbitrary remove.
Comment 15 Kentaro Hara 2012-07-24 00:29:26 PDT
Created attachment 153976 [details]
Patch
Comment 16 Kentaro Hara 2012-07-24 00:29:41 PDT
(In reply to comment #14)
>      if( stringImpl == m_lastStringImpl )
>         m_lastStringImpl = 0;
> 
> Then the last string cache will continue to work after an arbitrary remove.

Good point!. Fixed.
Comment 17 Adam Barth 2012-07-24 10:24:13 PDT
Comment on attachment 153976 [details]
Patch

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

> Source/WebCore/ChangeLog:40
> +        Without 'm_lastStringImpl = 0', already disposed m_lastV8String can be used
> +        in v8ExternalString(). This was a cause of the crashes of r122614.

Ah.  Interesting.
Comment 18 WebKit Review Bot 2012-07-24 11:43:14 PDT
Comment on attachment 153976 [details]
Patch

Clearing flags on attachment: 153976

Committed r123500: <http://trac.webkit.org/changeset/123500>
Comment 19 WebKit Review Bot 2012-07-24 11:43:19 PDT
All reviewed patches have been landed.  Closing bug.