Bug 104921

Summary: [V8] Regression causing DOM objects to have unstable NPObject* references with v8 bindings
Product: WebKit Reporter: Matthew Dempsky <mdempsky>
Component: WebCore Misc.Assignee: Matthew Dempsky <mdempsky>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, darin, dglazkov, haraken, japhet, ossy, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 105266    
Bug Blocks:    
Attachments:
Description Flags
Fix for NPV8Object regression
none
Patch
none
Patch
none
Patch
none
Patch none

Matthew Dempsky
Reported 2012-12-13 09:12:32 PST
Created attachment 179282 [details] Fix for NPV8Object regression For details, see crbug.com/165307. I've identified this WebKit patch as the issue: http://trac.webkit.org/changeset?format=diff&new=135804&old=135803&new_path=%2Ftrunk&old_path=%2Ftrunk In particular, in the "iter != v8NPObjectMap->end()" case, if the 'for' loop terminates without returning, then objectVector will not be set, and the created NPObject pointer will not be saved and the newly created NPObject will not be saved for future reuse. I also notice that v8NPObjectMap is keyed off Object::GetIdentityHash(), but the V8 Object documentation explicitly states that identity hashes are not unique: http://code.google.com/p/v8/source/browse/trunk/include/v8.h#1702 So I believe this code: if (v8npObject->rootObject == root) { ASSERT(v8npObject->v8Object == object); should also be changed to: if (v8npObject->v8Object == object && v8npObject->rootObject == root) { See attached patch.
Attachments
Fix for NPV8Object regression (1.22 KB, patch)
2012-12-13 09:12 PST, Matthew Dempsky 		no flags
DetailsFormatted DiffDiff
Patch (2.43 KB, patch)
2012-12-17 12:49 PST, Matthew Dempsky 		no flags
DetailsFormatted DiffDiff
Patch (4.16 KB, patch)
2012-12-17 14:51 PST, Matthew Dempsky 		no flags
DetailsFormatted DiffDiff
Patch (4.77 KB, patch)
2012-12-17 16:21 PST, Matthew Dempsky 		no flags
DetailsFormatted DiffDiff
Patch (4.76 KB, patch)
2012-12-17 16:24 PST, Matthew Dempsky 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Matthew Dempsky
Comment 1 2012-12-17 12:49:38 PST
Created attachment 179782 [details] Patch
Matthew Dempsky
Comment 2 2012-12-17 12:55:30 PST
haraken@ or japhet@ - can you r? cq? Thanks!
Matthew Dempsky
Comment 3 2012-12-17 12:59:26 PST
Comment on attachment 179782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179782&action=review > Source/WebCore/ChangeLog:17 > + No tests. Testing requires a custom plugin to compare NPObject* for FWIW, I've manually tested that this fix works for Chromium with the GWT Developer Mode plugin. However, I don't know how to build a custom NPAPI plugin just for the WebKit LayoutTests. If someone can give me clear instructions on how to do that, I'm happy to try that.
Matthew Dempsky
Comment 4 2012-12-17 14:51:54 PST
Created attachment 179807 [details] Patch
WebKit Review Bot
Comment 5 2012-12-17 15:37:45 PST
Comment on attachment 179807 [details] Patch Attachment 179807 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15364940 New failing tests: plugins/npruntime/embed-property-equality.html
Matthew Dempsky
Comment 6 2012-12-17 15:39:40 PST
Sorry, I'm still working on fixing the patch. There's a whitespace issue that caused it to fail, but more egregiously, it doesn't actually test the bug here since it passes even without my fix. I'm currently working on a new patch to add another method to TestNetscapePlugIn that will let me actually exercise the bug.
Kentaro Hara
Comment 7 2012-12-17 15:39:59 PST
Comment on attachment 179807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179807&action=review The change looks reasonable to me. I'll r+ it once you fixed the test result. > LayoutTests/ChangeLog:11 > + * plugins/npruntime/embed-property-equality.html: Test two plugin instances. You need to update the test result. You can update the test result by: $ ./webkit/tools/layout_tests/run_webkit_tests.py --reset-results --debug plugins/npruntime/embed-property-equality.html (--debug is not needed if you're building in the Release mode.)
Matthew Dempsky
Comment 8 2012-12-17 16:21:02 PST
Created attachment 179826 [details] Patch
Kentaro Hara
Comment 9 2012-12-17 16:23:18 PST
Comment on attachment 179826 [details] Patch Looks OK
Matthew Dempsky
Comment 10 2012-12-17 16:24:00 PST
Created attachment 179827 [details] Patch
Matthew Dempsky
Comment 11 2012-12-17 16:24:54 PST
Comment on attachment 179827 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179827&action=review > LayoutTests/plugins/npruntime/embed-property-iframe-equality.html:4 > +<iframe srcdoc="<script>parent.plugin.remember(parent.document)</script>"></iframe> Sorry, made one minor fix to here. Only need one argument for remember(), not two.
WebKit Review Bot
Comment 12 2012-12-17 17:44:26 PST
Comment on attachment 179827 [details] Patch Clearing flags on attachment: 179827 Committed r137964: <http://trac.webkit.org/changeset/137964>
WebKit Review Bot
Comment 13 2012-12-17 17:44:30 PST
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 14 2012-12-18 01:57:36 PST
(In reply to comment #12) > (From update of attachment 179827 [details]) > Clearing flags on attachment: 179827 > > Committed r137964: <http://trac.webkit.org/changeset/137964> The new test fails on WK2 platforms - see https://bugs.webkit.org/show_bug.cgi?id=105266 for details.
Matthew Dempsky
Comment 15 2012-12-18 10:18:17 PST
What's the proper process to request this patch be considered for merging into branches/chromium/1312 and branches/chromium/1359? The regression was introduced in r135804, which was merged into branches/chromium/1312 as r136064, and was also included in the base version for branches/chromium/1359 (which branched from r137490).
Kentaro Hara
Comment 16 2012-12-18 16:26:53 PST
(In reply to comment #15) > What's the proper process to request this patch be considered for merging into branches/chromium/1312 and branches/chromium/1359? The regression was introduced in r135804, which was merged into branches/chromium/1312 as r136064, and was also included in the base version for branches/chromium/1359 (which branched from r137490). Sent you a private email.
Matthew Dempsky
Comment 17 2012-12-19 22:51:16 PST
FYI, I've filed https://bugs.webkit.org/show_bug.cgi?id=105495 with more variations on this issue. I've already written LayoutTests to demonstrate the bugs, but I don't have fixes for them yet.
Note You need to log in before you can comment on or make changes to this bug.