Summary: | [V8] String wrappers should be Independent | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kentaro Hara <haraken> | ||||||||
Component: | WebCore JavaScript | Assignee: | Kentaro Hara <haraken> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, antonm, arv, dcarney, japhet, jochen, rniwa, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 91317 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Kentaro Hara
2012-07-13 08:41:10 PDT
Created attachment 152269 [details]
Patch
Comment on attachment 152269 [details]
Patch
Ok.
Comment on attachment 152269 [details] Patch Clearing flags on attachment: 152269 Committed r122614: <http://trac.webkit.org/changeset/122614> All reviewed patches have been landed. Closing bug. 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 Shall we revert the change and study the crashes? (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. Confirmed. Re-opened since this is blocked by 91317 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: (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. > 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.
Created attachment 153973 [details]
Patch
(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. Created attachment 153976 [details]
Patch
(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 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 on attachment 153976 [details] Patch Clearing flags on attachment: 153976 Committed r123500: <http://trac.webkit.org/changeset/123500> All reviewed patches have been landed. Closing bug. |