Bug 41292 – [V8]When a NPAPI plugin calls NPN_Evaluate to execute script in browser, if the call is initiated by user, allow the popup

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Johnny(Jianning) Ding]

Now YouTube's videos all have new new-window button on player (see the first attachment). When clicking that button on Chromium windows, nothing happened. The Chromium bug is here: http://crbug.com/43547

After investigation, I found the window.open was triggered in the script calling from Plugin side by using NPN_Evaluate interface.
In Chromium, the NPN_Evaluate called _NPN_EvaluateHelper(V8NPObject.cpp) to execute the script with passing a additional parameter "popupsAllowed".
The parameter "popupsAllowed" was from PluginInstance::popups_allowed() which was set when the input message could be considered as user gesture event (WebPluginDelegateImpl::IsUserGesture/WebPluginDelegateImpl::IsUserGestureMessage).

In chromium windows version, the input events to windowed plugin (for the youtube case) were handled by the plugin window, so the UserGestureIndicator had no chance to be set to right value:DefinitelyProcessingUserGesture. Also we didn't set right value for UserGestureIndicator when calling function _NPN_EvaluateHelper with "popupsAllowed":true, so the popup window was not considered as user initiated.

But in Safari Mac version, all input events even the input events to plugin are delivered by webview and handled by event handlers in WebCore/page/EventHandler.cpp) in which UserGestureIndicator is set to DefinitelyUserGesture, so the popup window is treated as user initiated.

To fix this issue on chromium, we may need to set UserGestureIndicator to DefinitelyProcessingUserGesture in function:_NPN_EvaluateHelper when popupsAllowed is true.

Any suggestions?

[Johnny(Jianning) Ding]

Created attachment 59918 [details]
youtube new-window feature illustration, original uploaded by dogantuncer on http://crbug.com/43547

[Johnny(Jianning) Ding]

Created attachment 59922 [details]
patch v1 to set right value on UserGestureIndicator to indicate whether the NPN_Evaluate allows popup window or not.

I didn't add layout test on this change. Will think about the test when I figure out why Chroimum Mac is OK for youtube case without this change.

[WebKit Review Bot]

Attachment 59922 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/v8/NPV8Object.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 59922 [details]
patch v1 to set right value on UserGestureIndicator to indicate whether the NPN_Evaluate allows popup window or not.

This patch looks "obviously" correct.  We just need a test.  You should be able to write one using the WebKit test plugin.  (Also, please fix style-bots nit...  We use case-sensitive alphabetical includes for whatever reason.)

[Johnny(Jianning) Ding]

Created attachment 60121 [details]
patch V2, add a test

[WebKit Review Bot]

Attachment 60121 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/PluginObject.cpp:983:  Use 0 instead of NULL.  [readability/null] [5]
WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/main.cpp:116:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/main.cpp:118:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/main.cpp:297:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/main.cpp:307:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/main.cpp:383:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/main.cpp:406:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 7 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Johnny(Jianning) Ding]

Created attachment 60124 [details]
Patch V2 with fixing style warnings, add a plugin test

[Adam Barth]

Comment on attachment 60124 [details]
Patch V2 with fixing style warnings, add a plugin test

Nice test.  Thanks.

WebCore/bindings/v8/NPV8Object.cpp:282
 +      // Initialize UserGestureIndicator to right value.
This comment is redundant and probably should be removed.

[Johnny(Jianning) Ding]

Hi Adam,

Thanks, should I upload a new patch which already removed the redundant comment?

By the way, since Chromium has its own DumpRenderTree utilities, this test will break chromium test.
I will file a chromium bug to implement same logic in Chromium's TestNetscapePlugin on crbug.com and provide a patch to change LayoutTest/platform/chromium/test_expectations.txt in this bug thread after this change is landed. Or maybe I should do this now in this patch. What do you suggest?

[Adam Barth]

> Thanks, should I upload a new patch which already removed the redundant comment?

Sure, if you like.  You don't need to get it reviewed again.  Uploading a new patch will let us land you patch with the commit-queue.

> By the way, since Chromium has its own DumpRenderTree utilities, this test will break chromium test.
> I will file a chromium bug to implement same logic in Chromium's TestNetscapePlugin on crbug.com and provide a patch to change LayoutTest/platform/chromium/test_expectations.txt in this bug thread after this change is landed. Or maybe I should do this now in this patch. What do you suggest?

You probably should change test_expectations.txt in this patch so that the canary bot won't turn red after landing your patch.

[Johnny(Jianning) Ding]

Created attachment 60182 [details]
patch v2 with removing redundant comment and skip the test on chromium

Thanks Adam. 

The new patch v2 removed redundant comment and change the chromium's test_expectations.txt to skip the test on chromium.
Since a new file was modified in this this patch, I still add review flag for this patch. Please take a look.

[Adam Barth]

Comment on attachment 60182 [details]
patch v2 with removing redundant comment and skip the test on chromium

great.  thanks

[Johnny(Jianning) Ding]

Hi Adam,

Would you or someone else please help on landing this change?
Thanks!

[Adam Barth]

Comment on attachment 60182 [details]
patch v2 with removing redundant comment and skip the test on chromium

sure

[WebKit Commit Bot]

Comment on attachment 60182 [details]
patch v2 with removing redundant comment and skip the test on chromium

Rejecting patch 60182 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1
Last 500 characters of output:
Hunk #1 FAILED at 2891.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej
patching file LayoutTests/platform/gtk/Skipped
Hunk #1 succeeded at 3395 (offset 1 line).
patching file LayoutTests/platform/qt/Skipped
Hunk #1 succeeded at 457 (offset 35 lines).
patching file LayoutTests/platform/win/Skipped
patching file LayoutTests/plugins/plugin-initiate-popup-window-expected.txt
patching file LayoutTests/plugins/plugin-initiate-popup-window.html

Full output: http://webkit-commit-queue.appspot.com/results/3480036

[Eric Seidel (no email)]

Comment on attachment 60124 [details]
Patch V2 with fixing style warnings, add a plugin test

Cleared Adam Barth's review+ from obsolete attachment 60124 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Johnny(Jianning) Ding]

Adam, Eric,

I just checked the log of commit failure, it's because the WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj was recently renamed to TestNetscapePlugIn and chromium's expectation.txt was changed.

I am creating a new patch according to those changes and will send for review. Please add it to commit queue when the patch gets passed.

Thanks!

[Johnny(Jianning) Ding]

Created attachment 61031 [details]
patch v2 to adjust to the change of TestNetscapePlugin renaming and Chromium's expectation.txt

[Johnny(Jianning) Ding]

Created attachment 61217 [details]
patch V2

just found conflict on chromium/text_expectation.txt, resolved it and created the latest patch V2.

[Johnny(Jianning) Ding]

Hi Adam,

Since platform/chromiun/test_expectation.txt was changed very frequently, juts found conflict again.
Do you know anyway I can use to let  my patch have no conflict with that file, otherwise I can not land this patch via the commit queue.

Thanks!

[Adam Barth]

Comment on attachment 61217 [details]
patch V2

Let's try one more time.  If it doesn't work, we can land it manually.

[WebKit Commit Bot]

Comment on attachment 61217 [details]
patch V2

Rejecting patch 61217 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1
Last 500 characters of output:
nk #1 succeeded at 2993 (offset 3 lines).
Hunk #2 FAILED at 3011.
1 out of 2 hunks FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej
patching file LayoutTests/platform/gtk/Skipped
patching file LayoutTests/platform/qt/Skipped
patching file LayoutTests/platform/win/Skipped
Hunk #1 succeeded at 804 (offset -1 lines).
patching file LayoutTests/plugins/plugin-initiate-popup-window-expected.txt
patching file LayoutTests/plugins/plugin-initiate-popup-window.html

Full output: http://webkit-commit-queue.appspot.com/results/3481249

[Johnny(Jianning) Ding]

The commit is failed again because of the conflict in test_expectation.txt.

Seems like we have to quickly resolve the conflict based on the latest chromium/test_expectation.txt and manually land it.

Thanks Adam!

By the way, I already had 10+ non-trivial CL(plus my previous account: johnnyding.webkit@gmail). I will apply the committer privilege when I have 20.

[Adam Barth]

Committed r63352: <http://trac.webkit.org/changeset/63352>

[Adam Barth]

I'm rolling out this patch because it broke two tests on snow leopard:

plugins/plugin-initiate-popup-window.html
plugins/plugin-javascript-access.html

http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r63408%20(13677)/plugins/plugin-initiate-popup-window-pretty-diff.html
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r63408%20(13677)/plugins/plugin-javascript-access-pretty-diff.html

[Johnny(Jianning) Ding]

(In reply to comment #25)
> I'm rolling out this patch because it broke two tests on snow leopard:
> 
> plugins/plugin-initiate-popup-window.html
> plugins/plugin-javascript-access.html
> 
> http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r63408%20(13677)/plugins/plugin-initiate-popup-window-pretty-diff.html
> http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r63408%20(13677)/plugins/plugin-javascript-access-pretty-diff.html

I see, my mac is leopard not snow leopard.
Looks the diff files, seems like we need to generate the individual expected file for leopard and snow leopard.

[Adam Barth]

> I see, my mac is leopard not snow leopard.
> Looks the diff files, seems like we need to generate the individual expected file for leopard and snow leopard.

Why does leopard and snow leopard have different results?

[Adam Barth]

It looks like state is leaking from one test to another.  Notice that the console messages disappear from one and appear in the other.  It's more complex than just rebaselining.

[Adam Barth]

rolled out in http://trac.webkit.org/changeset/63409

[Johnny(Jianning) Ding]

(In reply to comment #28)
> It looks like state is leaking from one test to another.  Notice that the console messages disappear from one and appear in the other.  It's more complex than just rebaselining.

you are right, it's stranger. I will find a snow leopard machine to debug this.

[Johnny(Jianning) Ding]

My preliminary guess is when running plugin-initiate-popup-window.html on snow leopard,
when calling notifyDone, the test plugin still had focus. But after calling notifyDone, the document was unloading to cause plugin to lost focus, so the function plugLog sent 'LostFocus' message to dumpDOMTree. 
However, notifyDone already called Dump() to generate actual result file, so the console messages disappeared from plugin-initiate-popup-window.html and appeared in the other test:plugins/plugin-javascript-access.html.

[Adam Barth]

Comment on attachment 61217 [details]
patch V2

Looks like we'll need a new patch here.

[Maciej Stachowiak]

A bug was filed in Radar that mentioned this bugzilla bug, but I don't understand the relationship:

<rdar://problem/8215635> WebKitPluginHost didn't pass right value to nestcape plugin for allowPopup flag

Can someone explain the relationship? Is there an actual symptom in SnowLeopard Safari?

[Johnny(Jianning) Ding]

Created attachment 67673 [details]
layout test case

(In reply to comment #33)
> A bug was filed in Radar that mentioned this bugzilla bug, but I don't understand the relationship:
> 
> <rdar://problem/8215635> WebKitPluginHost didn't pass right value to nestcape plugin for allowPopup flag
> 
> Can someone explain the relationship? Is there an actual symptom in SnowLeopard Safari?

Hi Maciej,
I filed the Radar bug because of failure of the attached layout test. I am going to add this layout test in the patch for this bug.

Starting from snow leopard, the npapi plugin is run under "WebKitPluginHost.app" process, and WebKit relies on the allowPopups flag passed from "WebKitPluginHost.app" RPC to control to whether block the popups initiated by plugin or not. 
When debugging with XCode, I found the allowPopups flags from "WebKitPluginHost.app" was not right. (Please take a look the attached debug snapshot).
I hope my explanation answers your question.

[Johnny(Jianning) Ding]

Created attachment 67674 [details]
debug snapshot

[Johnny(Jianning) Ding]

Created attachment 67689 [details]
patch v3

Hi Adam,
I have create a new patch. It sets the popupAllowed flag passed from plugin to current execution frame, so WebKit can get right gesture state for the popups initiated by users from plugin side.

[Adam Barth]

Comment on attachment 67689 [details]
patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=67689&action=prettypatch

This looks good to me, but I'm not an expert on the plugin code.  Might be worth having a plug-in expert take a look too.

> WebCore/bindings/v8/NPV8Object.cpp:285
> +    ASSERT(frame);
This assert seems unneeded.  We'll crash on the next line anyway.

> WebKit/mac/ChangeLog:9
> +        execution frame, so WeKit can always get right gesture state on Mac.
WeKit -> WebKit

> WebKit/mac/ChangeLog:10
> +        From Mac Snow Leopard, the plugin is run under "WebKitPluginHost,app"
.app ?

[Johnny(Jianning) Ding]

(In reply to comment #37)
> (From update of attachment 67689 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67689&action=prettypatch
> 
> This looks good to me, but I'm not an expert on the plugin code.  Might be worth having a plug-in expert take a look too.

Thanks, Adam! Would you please add or recommend a plug-in expert t review this patch?

I will upload a landing patch (remove the typo and unnecessary code according to comments of you and the plugin-expert) after the plug-in expert reviews this patch.

If he/she is OK with this patch, would you please help me to land this patch? (may need to manually land since the chromium/test-expectations.txt often changes to cause conflict.)

[Adam Barth]

I'm not sure who would be the best plug-in expert.  Maybe ask on #webkit.

[Adam Barth]

Committed r68630: <http://trac.webkit.org/changeset/68630>

[Eric Seidel (no email)]

plugins/plugin-initiate-popup-window.html is broken on Snow Leopard and has been since checkin.

[Adam Barth]

(In reply to comment #41)
> plugins/plugin-initiate-popup-window.html is broken on Snow Leopard and has been since checkin.

Not sure why sheriffbot didn't complain.  In any case, I've update the expected results.

[Robert Hogan]

Created attachment 91925 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 91925 [details]
Patch

Cleared review? from attachment 91925 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Robert Hogan]

Reopening for Qt fix.

[Robert Hogan]

Created attachment 94457 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 94457 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94457&action=review

> LayoutTests/ChangeLog:10
> +        * platform/qt/plugins/plugin-initiate-popup-window-expected.txt: Copied from LayoutTests/platform/mac/plugins/plugin-initiate-popup-window-expected.txt.

Can we put the results next to the test instead? In what way are they platform-specific?

> Source/WebCore/ChangeLog:9
> +        If the event is from a user gesture, allow popups.

...and the plugin doesn't support the popup-related NPAPI calls, right? You should mention that here, too.

> Source/WebCore/plugins/qt/PluginViewQt.cpp:390
> +    if (m_plugin->pluginFuncs()->version < NPVERS_HAS_POPUPS_ENABLED_STATE
> +        && (event.type == ButtonRelease || event.type == 3 /*KeyRelease*/)) {

Is there really no constant for KeyRelease?

[Robert Hogan]

(In reply to comment #47)
> (From update of attachment 94457 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94457&action=review
> 
> > LayoutTests/ChangeLog:10
> > +        * platform/qt/plugins/plugin-initiate-popup-window-expected.txt: Copied from LayoutTests/platform/mac/plugins/plugin-initiate-popup-window-expected.txt.
> 
> Can we put the results next to the test instead? In what way are they platform-specific?

The plugin-log lines make this and most tests that use them platform-specific it seems.

> 
> > Source/WebCore/ChangeLog:9
> > +        If the event is from a user gesture, allow popups.
> 
> ...and the plugin doesn't support the popup-related NPAPI calls, right? You should mention that here, too.

Updated.

> 
> > Source/WebCore/plugins/qt/PluginViewQt.cpp:390
> > +    if (m_plugin->pluginFuncs()->version < NPVERS_HAS_POPUPS_ENABLED_STATE
> > +        && (event.type == ButtonRelease || event.type == 3 /*KeyRelease*/)) {
> 
> Is there really no constant for KeyRelease?

There is, but Qt overwrites it.

[Robert Hogan]

Committed r87429: <http://trac.webkit.org/changeset/87429>

[Adam Roben (:aroben)]

Comment on attachment 94457 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94457&action=review

>>> LayoutTests/ChangeLog:10
>>> +        * platform/qt/plugins/plugin-initiate-popup-window-expected.txt: Copied from LayoutTests/platform/mac/plugins/plugin-initiate-popup-window-expected.txt.
>> 
>> Can we put the results next to the test instead? In what way are they platform-specific?
> 
> The plugin-log lines make this and most tests that use them platform-specific it seems.

Can't we just require that all platforms make the test plugin log the same messages? We already require that for DRT.

[Adam Roben (:aroben)]

Why does this bug have "[V8]" in the title? Is it really V8-specific?

[Robert Hogan]

(In reply to comment #51)
> Why does this bug have "[V8]" in the title? Is it really V8-specific?

The original patch on this bug introduced the test that my patch fixes for Qt. At the time I thought it made sense to keep them under the same bug, but I now think it's just confusing for someone looking at the bug later.

[Robert Hogan]

(In reply to comment #50)
> (From update of attachment 94457 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94457&action=review
> 
> >>> LayoutTests/ChangeLog:10
> >>> +        * platform/qt/plugins/plugin-initiate-popup-window-expected.txt: Copied from LayoutTests/platform/mac/plugins/plugin-initiate-popup-window-expected.txt.
> >> 
> >> Can we put the results next to the test instead? In what way are they platform-specific?
> > 
> > The plugin-log lines make this and most tests that use them platform-specific it seems.
> 
> Can't we just require that all platforms make the test plugin log the same messages? We already require that for DRT.

The order in which the plugin receives events and the line numbers associated with the event seems to differ across platforms.

For example Qt vs Chromium-Win:


< CONSOLE MESSAGE: line 17: window count: 1                                                                                                  
< CONSOLE MESSAGE: line 0: PLUGIN: getFocusEvent                                                                                             
< CONSOLE MESSAGE: line 0: PLUGIN: mouseDown at (12, 12)                                                                                     
< CONSOLE MESSAGE: line 30: window count: 2                                                                                                  
< CONSOLE MESSAGE: line 0: PLUGIN: mouseUp at (12, 12)                                                                                       
< CONSOLE MESSAGE: line 0: PLUGIN: loseFocusEvent                                                                                            
< CONSOLE MESSAGE: line 0: PLUGIN: getFocusEvent                                                                                             
< CONSOLE MESSAGE: line 0: PLUGIN: mouseDown at (52, 12)
< CONSOLE MESSAGE: line 0: PLUGIN: mouseUp at (52, 12)
< CONSOLE MESSAGE: line 0: PLUGIN: keyDown 'p'
< CONSOLE MESSAGE: line 52: window count: 3
< CONSOLE MESSAGE: line 0: PLUGIN: keyUp 'p'
< 
< 
< Specify a script and a mouse/keyboard event to the plugin. The specified script will be evaluated in the browser when the specified event is received by the plugin. The test is for bug https://bugs.webkit.org/show_bug.cgi?id=41292.
< Opening window by mouse down is PASSED
< Opening window by key down is PASSED
---
> CONSOLE MESSAGE: line 0: PLUGIN: event 71
> CONSOLE MESSAGE: line 0: PLUGIN: event 71
> CONSOLE MESSAGE: line 17: window count: 1
> CONSOLE MESSAGE: line 21: PLUGIN: getFocusEvent
> CONSOLE MESSAGE: line 21: PLUGIN: mouseDown at (20, 20)
> CONSOLE MESSAGE: line 30: window count: 2
> CONSOLE MESSAGE: line 22: PLUGIN: mouseUp at (20, 20)
> CONSOLE MESSAGE: line 42: PLUGIN: loseFocusEvent
> CONSOLE MESSAGE: line 42: PLUGIN: getFocusEvent
> CONSOLE MESSAGE: line 42: PLUGIN: mouseDown at (60, 60)
> CONSOLE MESSAGE: line 43: PLUGIN: mouseUp at (60, 60)
> CONSOLE MESSAGE: line 44: PLUGIN: keyDown 'P'
> CONSOLE MESSAGE: line 52: window count: 3
> CONSOLE MESSAGE: line 44: PLUGIN: keyUp 'P'
> 

Qt vs Mac:
12a13
> CONSOLE MESSAGE: line 0: PLUGIN: loseFocusEvent

I've opened a bug for this : https://bugs.webkit.org/show_bug.cgi?id=61566

[Johnny(Jianning) Ding]

(In reply to comment #51)
> Why does this bug have "[V8]" in the title? Is it really V8-specific?

In the original bug, V8 version ScriptController:::processingUserGesture doesn't consider the allowPopups flag passed from plugin side, so the original patch forced on solving that issue, what's why it has [V8] in the title.