Bug 105495

Summary: [V8] More unstable NPObject* references for V8 script objects
Product: WebKit Reporter: Matthew Dempsky <mdempsky>
Component: WebCore Misc.Assignee: Adam Barth <abarth>
Status: RESOLVED INVALID    
Severity: Normal CC: andersca, dglazkov, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 106813    
Bug Blocks:    
Attachments:
Description Flags
Additional WebKit LayoutTests demonstrating unstable NPObject* values for V8 script objects
webkit.review.bot: commit-queue-
Proof of concept fixes for new tests none

Matthew Dempsky
Reported 2012-12-19 21:42:34 PST
Created attachment 180276 [details] Additional WebKit LayoutTests demonstrating unstable NPObject* values for V8 script objects As a followup to https://bugs.webkit.org/show_bug.cgi?id=104921, attached is a patch that adds two more LayoutTests to demonstrate further cases where objects that are treated as equivalent by JavaScript (i.e., according to '===') end up with distinct NPObject* values when passed to an NPAPI plugin. The first test (embed-property-iframe-equality-2.html) tests where the same DOM object is passed to an NPAPI plugin from two different contexts (the main page and a child iframe). Even though each reference is to the same DOM object, they get converted to distinct NPObject* values. - I believe this is due to npCreateV8ScriptObject only returning saved NPObject* values that match both v8Object and rootObject. If I remove the second half of the "v8npObject->v8Object == object && v8npObject->rootObject == root" test, then this test passes, but I'm not sure that's a complete or correct fix. The second test (embed-property-iframe-equality-3.html) tests where a DOM object attached to a child iframe (the child iframe's 'window' object) is stashed in the NPAPI plugin, the iframe is removed (so the creation context goes away), and then two references to the same object are passed back into the NPAPI plugin. Again, even though the references are to the same DOM object, they get converted to distinct NPObject* values. - I believe this is due to npCreateV8ScriptObject only saving NPObject* values for V8 objects that still have a "V8PerContextData". Once the iframe is gone, there's no V8PerContextData anymore, so each reference gets a completely new NPObject* value. I suspect saving the data elsewhere or preventing the V8PerContextData from getting deleted when there are still live references to the CreationContext would fix this test, but I don't know for sure. Both of these issues could adversely affect the GWT Developer Mode plugin just like 104921 did, so I hope they can be fixed, but I don't know of any users being affected by these issues at present.
Attachments
Additional WebKit LayoutTests demonstrating unstable NPObject* values for V8 script objects (4.16 KB, patch)
2012-12-19 21:42 PST, Matthew Dempsky 		webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
Proof of concept fixes for new tests (9.94 KB, patch)
2012-12-20 13:50 PST, Matthew Dempsky 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
WebKit Review Bot
Comment 1 2012-12-19 21:45:50 PST
Attachment 180276 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/plugins/npruntime/embed-proper..." exit_code: 1 Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:198: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Review Bot
Comment 2 2012-12-19 22:21:47 PST
Comment on attachment 180276 [details] Additional WebKit LayoutTests demonstrating unstable NPObject* values for V8 script objects Attachment 180276 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15401886 New failing tests: plugins/npruntime/embed-property-iframe-equality-2.html plugins/npruntime/embed-property-iframe-equality-3.html
Matthew Dempsky
Comment 3 2012-12-20 13:50:04 PST
Created attachment 180403 [details] Proof of concept fixes for new tests
Matthew Dempsky
Comment 4 2012-12-20 13:59:40 PST
I attached a patch that seems to fix both of these bugs, but I'm not confident it doesn't introduce further issues. It makes a few changes: 1. It decouples V8PerContextData's lifetime from V8DOMWindowShell. Instead, I changed V8PerContextData to keep a weak reference to its Context, and to delete itself when the context gets garbage collected. 2. Instead of needing to save a DOMWindow* in the NPV8Object struct, I take advantage of the fact that npp->ndata is actually a PluginView*, and we can use that to get to the plugin element's Frame* instead. (Tangentially, this fixes a TODO mentioned in NPV8Object.cpp.) This means npCreateV8ScriptObject() no longer has any reason to worry about matching on rootObject when creating a NPObject*. 3. The chromium port's public API exposes a "v8::Handle<v8::Value> WebBindings::toV8Value(const NPVariant* variant)" method that currently relies on NPV8Object's rootObject member and it doesn't accept an NPP argument. I haven't found any evidence that this method is actually used by Chromium, so I think it should just be removed or an NPP argument added.
Matthew Dempsky
Comment 5 2012-12-20 15:24:35 PST
Hm, I'm not certain I can actually rely on npp->ndata like that for two reasons: 1) A lot of places in the Chromium code base appear to call the WebBindings methods (which then forward to the _NPN_*() functions) with npp==NULL, and 2) Chromium seems to create an NPP object with ndata set to PluginInstance instead of PluginView.
Matthew Dempsky
Comment 6 2013-01-11 09:22:51 PST
Adam, any thoughts on these issues?
Adam Barth
Comment 7 2013-01-11 10:28:43 PST
> 1. It decouples V8PerContextData's lifetime from V8DOMWindowShell. Instead, I changed V8PerContextData to keep a weak reference to its Context, and to delete itself when the context gets garbage collected. Wow! I've wanted to do that for a long time. > 2. Instead of needing to save a DOMWindow* in the NPV8Object struct, I take advantage of the fact that npp->ndata is actually a PluginView*, and we can use that to get to the plugin element's Frame* instead. (Tangentially, this fixes a TODO mentioned in NPV8Object.cpp.) This means npCreateV8ScriptObject() no longer has any reason to worry about matching on rootObject when creating a NPObject*. > > 3. The chromium port's public API exposes a "v8::Handle<v8::Value> WebBindings::toV8Value(const NPVariant* variant)" method that currently relies on NPV8Object's rootObject member and it doesn't accept an NPP argument. I haven't found any evidence that this method is actually used by Chromium, so I think it should just be removed or an NPP argument added. Great! Let's remove it.
Adam Barth
Comment 8 2013-01-11 10:29:43 PST
Comment on attachment 180276 [details] Additional WebKit LayoutTests demonstrating unstable NPObject* values for V8 script objects These tests look fine, but we'll probably want to land them with the fix.
Adam Barth
Comment 9 2013-01-11 10:33:20 PST
Comment on attachment 180403 [details] Proof of concept fixes for new tests View in context: https://bugs.webkit.org/attachment.cgi?id=180403&action=review > Source/WebCore/bindings/v8/V8DOMWindowShell.h:105 > - OwnPtr<V8PerContextData> m_perContextData; > + V8PerContextData* m_perContextData; I wonder if we can remove this member entirely. If we need the V8PerContextData, we can get it from the m_context below. > Source/WebCore/bindings/v8/WorkerScriptController.cpp:-89 > - m_perContextData.clear(); Can we 0 out m_perContextData at this point? > Source/WebCore/bindings/v8/V8PerContextData.cpp:83 > + delete perContextData; You should also SetAlignedPointerInEmbedderData(v8ContextPerContextDataIndex, 0); here. We don't want anyone accessing the perContextData after it is deleted. > Source/WebCore/bindings/v8/NPV8Object.cpp:64 > + return view->parentFrame(); Can this ever return 0?
Adam Barth
Comment 10 2013-01-11 10:34:14 PST
There's a lot of good stuff in this patch. I would recommend splitting it into the three pieces you mention and landing them separately. In particular, the change to the lifetime of V8PerContextData is something that might have wide-reaching effects, so it would be good to land that in isolation from the rest of these changes.
Matthew Dempsky
Comment 11 2013-01-11 10:46:27 PST
Okay, I'll do the lifetime patch first since that's the most worrying issue IMO from the GWT DevMode plugin point-of-view. If I plan to stage the patches, should I file a new bug for each one (and mark them as blocking this one), or just attach them all to this one?
Adam Barth
Comment 12 2013-01-11 10:55:09 PST
> If I plan to stage the patches, should I file a new bug for each one (and mark them as blocking this one), or just attach them all to this one? We prefer to have one patch per bug. Marking them as blocking this bug is a good idea. Thanks!
Matthew Dempsky
Comment 13 2013-01-11 10:55:41 PST
Comment on attachment 180403 [details] Proof of concept fixes for new tests View in context: https://bugs.webkit.org/attachment.cgi?id=180403&action=review >> Source/WebCore/bindings/v8/V8DOMWindowShell.h:105 >> + V8PerContextData* m_perContextData; > > I wonder if we can remove this member entirely. If we need the V8PerContextData, we can get it from the m_context below. I think so. >> Source/WebCore/bindings/v8/WorkerScriptController.cpp:-89 >> - m_perContextData.clear(); > > Can we 0 out m_perContextData at this point? Probably, but we can probably just get rid of it too like in V8DOMWindowShell. >> Source/WebCore/bindings/v8/V8PerContextData.cpp:83 >> + delete perContextData; > > You should also SetAlignedPointerInEmbedderData(v8ContextPerContextDataIndex, 0); here. We don't want anyone accessing the perContextData after it is deleted. Okay, I'll fix that in the next patch. >> Source/WebCore/bindings/v8/NPV8Object.cpp:64 >> + return view->parentFrame(); > > Can this ever return 0? No clue. Like I mentioned in the bug, I'm not actually sure about the PluginView stuff anymore, since in the Chromium tree a lot of functions are called with npp==NULL. Also, in chrome/src/webkit/plugins/npapi/plugin_instance.cc, Chromium creates its own NPP object and sets npp->ndata to a webkit::npapi::PluginInstance*. I don't really understand the out-of-process plugin stuff, so I'm not sure what npp->ndata actually points to here.
Anders Carlsson
Comment 14 2013-09-01 10:34:50 PDT
V8 is gone.
Note You need to log in before you can comment on or make changes to this bug.