Bug 91251 – [V8] String wrappers should be Independent

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

Currently V8 String wrappers are not marked Independent. By marking them Independent, they can be reclaimed by the scavenger GC.

[Kentaro Hara]

Created attachment 152269 [details]
Patch

[Adam Barth]

Comment on attachment 152269 [details]
Patch

Ok.

[WebKit Review Bot]

Comment on attachment 152269 [details]
Patch

Clearing flags on attachment: 152269

Committed r122614: <http://trac.webkit.org/changeset/122614>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

It appears that two perf tests started crashing after this patch:
http://build.webkit.org/builders/Chromium%20Linux%20Release%20%28Perf%29/builds/4057/steps/perf-test/logs/stdio

Blame list:
http://build.webkit.org/builders/Chromium%20Linux%20Release%20%28Perf%29/builds/4057

[Adam Barth]

Shall we revert the change and study the crashes?

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Confirmed.

[WebKit Review Bot]

Re-opened since this is blocked by 91317

[Kentaro Hara]

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:

[Erik Arvidsson]

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

[Kentaro Hara]

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

[Kentaro Hara]

Created attachment 153973 [details]
Patch

[Dan Carney]

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

[Kentaro Hara]

Created attachment 153976 [details]
Patch

[Kentaro Hara]

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

[Adam Barth]

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.

[WebKit Review Bot]

Comment on attachment 153976 [details]
Patch

Clearing flags on attachment: 153976

Committed r123500: <http://trac.webkit.org/changeset/123500>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.