[V8] More unstable NPObject* references for V8 script objects

• Component: WebCore Misc.
• Status: RESOLVED INVALID

[Matthew Dempsky]

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.

[WebKit Review Bot]

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

Created attachment 180403 [details]
Proof of concept fixes for new tests

[Matthew Dempsky]

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]

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]

Adam, any thoughts on these issues?

[Adam Barth]

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

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]

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]

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

V8 is gone.