Bug 108182

Summary: REGRESSION (r141051): Broke plugin support on non-Mac WebKit2 Ports
Product: WebKit Reporter: Thiago Marcos P. Santos <tmpsantos>
Component: WebKit2Assignee: Thiago Marcos P. Santos <tmpsantos>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, andersca, cgarcia, cmarcelo, gns, hausmann, jturcotte, kenneth, menard, mrobinson, ossy, sam, webkit.review.bot, zarvai
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 107456, 79668, 108098    
Attachments:
Description Flags
Patch
none
Patch sam: review+

Description Thiago Marcos P. Santos 2013-01-29 05:21:50 PST
Failing tests on EFL bot:

  http/tests/plugins/create-v8-script-objects.html [ Failure ]
  http/tests/plugins/cross-frame-object-access.html [ Failure ]
  http/tests/plugins/get-url.html [ Failure ]
  http/tests/plugins/geturlnotify-from-npp-destroystream.html [ Failure ]
  http/tests/plugins/interrupted-get-url.html [ Failure ]
  http/tests/plugins/local-geturl-from-remote.html [ Failure ]
  http/tests/plugins/npapi-response-headers.html [ Failure ]
  http/tests/plugins/third-party-cookie-accept-policy.html [ Failure ]
  http/tests/security/contentSecurityPolicy/object-src-none-allowed.html [ Failure ]
  http/tests/security/cross-origin-plugin-allowed.html [ Failure ]
  http/tests/security/cross-origin-plugin-private-browsing-toggled-allowed.html [ Failure ]
  http/tests/security/storage-blocking-loosened-plugin.html [ Failure ]
  http/tests/security/storage-blocking-loosened-private-browsing-plugin.html [ Failure ]
  plugins/destroy-during-npp-new-object-with-fallback-content.html [ Failure ]
  plugins/destroy-during-npp-new.html [ Failure ]
  plugins/destroy-plugin-from-callback.html [ Failure ]
  plugins/destroy-stream-twice.html [ Failure ]
  plugins/document-open.html [ Failure ]
  plugins/embed-inside-object.html [ Failure ]
  plugins/embed-prefers-plugins-for-images.html [ Failure ]
  plugins/form-value.html [ Failure ]
  plugins/fullscreen-plugins-dont-reload.html [ Failure ]
  plugins/get-empty-url.html [ Failure ]
  plugins/get-file-url.html [ Failure ]
  plugins/get-javascript-url.html [ Failure ]
  plugins/get-url-notify-with-url-that-fails-to-load.html [ Failure ]
  plugins/get-url-with-iframe-target.html [ Failure ]
  plugins/get-url-with-javascript-destroying-plugin.html [ Failure ]
  plugins/get-url-with-javascript-url.html [ Failure ]
  plugins/get-user-agent-with-null-npp-from-npp-new.html [ Failure ]
  plugins/geturl-replace-query.html [ Failure ]
  plugins/inner-html-display-none.html [ Failure ]
  plugins/instance-available-before-stylesheets-loaded-object.html [ Failure ]
  plugins/instance-available-before-stylesheets-loaded.html [ Failure ]
  plugins/mouse-click-plugin-clears-selection.html [ Failure ]
  plugins/multiple-plugins.html [ Failure ]
  plugins/nested-plugin-objects.html [ Failure ]
  plugins/netscape-destroy-plugin-script-objects.html [ Failure ]
  plugins/netscape-dom-access.html [ Failure ]
  plugins/netscape-plugin-map-data-to-src.html [ Failure ]
  plugins/netscape-plugin-property-access-exception.html [ Failure ]
  plugins/npruntime/construct.html [ Failure ]
  plugins/npruntime/delete-plugin-within-invoke.html [ Failure ]
  plugins/npruntime/embed-property.html [ Failure ]
  plugins/npruntime/enumerate.html [ Failure ]
  plugins/npruntime/evaluate.html [ Failure ]
  plugins/npruntime/get-int-identifier-special-values.html [ Failure ]
  plugins/npruntime/get-property-return-value.html [ Failure ]
  plugins/npruntime/identifier-conversion.html [ Failure ]
  plugins/npruntime/invoke-browserfuncs.html [ Failure ]
  plugins/npruntime/invoke-default.html [ Failure ]
  plugins/npruntime/invoke-failure.html [ Failure ]
  plugins/npruntime/invoke.html [ Failure ]
  plugins/npruntime/npruntime-calls-with-null-npp.html [ Failure ]
  plugins/npruntime/npruntime.html [ Failure ]
  plugins/npruntime/object-from-destroyed-plugin-in-subframe.html [ Failure ]
  plugins/npruntime/object-from-destroyed-plugin.html [ Failure ]
  plugins/npruntime/overrides-all-properties.html [ Failure ]
  plugins/npruntime/remove-property.html [ Failure ]
  plugins/npruntime/round-trip-npobject.html [ Failure ]
  plugins/npruntime/throw-exception.html [ Failure ]
  plugins/object-embed-plugin-scripting.html [ Failure ]
  plugins/override-node-method.html [ Failure ]
  plugins/plugin-document-back-forward.html [ Failure ]
  plugins/plugin-remove-subframe.html [ Failure ]
  plugins/private-browsing-mode-2.html [ Failure ]
  plugins/private-browsing-mode.html [ Failure ]
  plugins/reloadplugins-no-pages.html [ Failure ]
  plugins/resize-from-plugin.html [ Failure ]
  plugins/return-error-from-new-stream-doesnt-invoke-destroy-stream.html [ Failure ]
  plugins/return-negative-one-from-write.html [ Failure ]
  plugins/return-npobject.html [ Failure ]
  plugins/script-object-invoke.html [ Failure ]
  plugins/update-widgets-crash.html [ Failure ]
  plugins/window-open.html [ Failure ]
Comment 1 Zoltan Arvai 2013-01-29 05:52:09 PST
Also on Qt5
http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Amazon%20EC2%29/r141093%20%2813137%29/results.html

Failing tests on Qt5-WK2:

plugins/destroy-during-npp-new-object-with-fallback-content.html
plugins/destroy-during-npp-new.html
plugins/destroy-plugin-from-callback.html
plugins/fullscreen-plugins-dont-reload.html
plugins/get-empty-url.html
plugins/get-user-agent-with-null-npp-from-npp-new.html
plugins/inner-html-display-none.html
plugins/instance-available-before-stylesheets-loaded-object.html
plugins/instance-available-before-stylesheets-loaded.html
plugins/multiple-plugins.html
plugins/nested-plugin-objects.html
plugins/netscape-destroy-plugin-script-objects.html
plugins/netscape-plugin-map-data-to-src.html
plugins/netscape-plugin-property-access-exception.html
plugins/npruntime/construct.html
plugins/npruntime/embed-property.html
plugins/npruntime/enumerate.html
plugins/npruntime/evaluate.html
plugins/npruntime/get-int-identifier-special-values.html
plugins/npruntime/get-property-return-value.html
plugins/npruntime/identifier-conversion.html
plugins/npruntime/invoke-browserfuncs.html
plugins/npruntime/invoke-default.html
plugins/npruntime/invoke-failure.html
plugins/npruntime/invoke.html
plugins/npruntime/npruntime-calls-with-null-npp.html
plugins/npruntime/npruntime.html
plugins/npruntime/overrides-all-properties.html
plugins/npruntime/remove-property.html
plugins/npruntime/round-trip-npobject.html
plugins/npruntime/throw-exception.html
plugins/object-embed-plugin-scripting.html
plugins/override-node-method.html
plugins/plugin-remove-subframe.html
plugins/private-browsing-mode-2.html
plugins/private-browsing-mode.html
plugins/reloadplugins-no-pages.html
plugins/return-error-from-new-stream-doesnt-invoke-destroy-stream.html
plugins/return-negative-one-from-write.html
plugins/return-npobject.html
plugins/script-object-invoke.html
plugins/update-widgets-crash.html
plugins/window-open.html

Timeouting tests on Qt5-WK2:

plugins/get-targeted-javascript-url.html
plugins/get-url-with-javascript-destroying-plugin.html
plugins/destroy-stream-twice.html
plugins/get-url-with-javascript-url.html
plugins/get-javascript-url.html
plugins/get-url-notify-with-url-that-fails-to-load.html
plugins/get-file-url.html
plugins/get-url-with-iframe-target.html
plugins/geturl-replace-query.html
Comment 2 Thiago Marcos P. Santos 2013-01-29 08:49:47 PST
Fix is on the way.
Comment 3 Thiago Marcos P. Santos 2013-01-29 09:49:39 PST
Created attachment 185255 [details]
Patch
Comment 4 Simon Hausmann 2013-01-29 10:26:43 PST
Comment on attachment 185255 [details]
Patch

Can any of the WebKit2 owners review this patch? It's a test regression fix.

This is one workable approach, the other would be to introduce a way of passing extra initialization data for the other ports.
Comment 5 Thiago Marcos P. Santos 2013-01-29 10:30:45 PST
(In reply to comment #4)
> (From update of attachment 185255 [details])
> Can any of the WebKit2 owners review this patch? It's a test regression fix.
> 
> This is one workable approach, the other would be to introduce a way of passing extra initialization data for the other ports.

The extra initialization data is only needed (so far) by Mac. This patch also moves it to inside the PLATFORM(MAC) boundaries.
Comment 6 Anders Carlsson 2013-01-29 10:37:38 PST
Comment on attachment 185255 [details]
Patch

This isn't the right approach. ProcessLauncher should support passing the extra initialization data along to the child process. See the part in createProcess in ProcessLauncherMac, where we iterate over the extraInitializationData map and create arguments.
Comment 7 Thiago Marcos P. Santos 2013-01-29 13:11:26 PST
(In reply to comment #6)
> (From update of attachment 185255 [details])
> This isn't the right approach. ProcessLauncher should support passing the extra initialization data along to the child process. See the part in createProcess in ProcessLauncherMac, where we iterate over the extraInitializationData map and create arguments.

That could be done (and was my initial approach to be honest), but wont solve the problem for all the !mac platforms at once, since each port has its own ProcessLauncher.

This is what motivated me to basically restoring the original behavior on this patch. It is also worth mentioning that extraInitializationData is only needed by Mac.

Could you please reconsider? Maybe we could land this patch and give time to each port migrate their ProcessLauncher to the solution you are suggesting.
Comment 8 Sam Weinig 2013-01-29 17:12:58 PST
(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 185255 [details] [details])
> > This isn't the right approach. ProcessLauncher should support passing the extra initialization data along to the child process. See the part in createProcess in ProcessLauncherMac, where we iterate over the extraInitializationData map and create arguments.
> 
> That could be done (and was my initial approach to be honest), but wont solve the problem for all the !mac platforms at once, since each port has its own ProcessLauncher.
> 
> This is what motivated me to basically restoring the original behavior on this patch. It is also worth mentioning that extraInitializationData is only needed by Mac.
> 
> Could you please reconsider? Maybe we could land this patch and give time to each port migrate their ProcessLauncher to the solution you are suggesting.

You should not feel compelled to fix this for all the ports.  As mentioned in the past, the intent of our new review model is to allow these exact type of change, so I we are not going to reconsider this patch.
Comment 9 Simon Hausmann 2013-01-30 01:29:15 PST
(In reply to comment #8)
> You should not feel compelled to fix this for all the ports. 

I think that we should try to make an effort to fix this for all the ports, because there is an opportunity of reducing or avoiding duplicated code.

Thiago, what if for Gtk/Efl/Qt we translate the extra initialization data dictionary into environment variables to the launched process and then in the launched process iterate over the environment and get the values back?

I guess that could be shared code because only PLATFORM(MAC) is using XPC and everyone else is using a regular child process.
Comment 10 Simon Hausmann 2013-01-30 01:50:35 PST
(In reply to comment #9)
> (In reply to comment #8)
> > You should not feel compelled to fix this for all the ports. 
> 
> I think that we should try to make an effort to fix this for all the ports, because there is an opportunity of reducing or avoiding duplicated code.
> 
> Thiago, what if for Gtk/Efl/Qt we translate the extra initialization data dictionary into environment variables to the launched process and then in the launched process iterate over the environment and get the values back?
> 
> I guess that could be shared code because only PLATFORM(MAC) is using XPC and everyone else is using a regular child process.

Oops, I just found out that on Windows environment variable names are case insensitive. That could be an issue. Perhaps we can agree on only using lower-case strings for the keys?
Comment 11 Thiago Marcos P. Santos 2013-01-30 08:01:03 PST
Created attachment 185507 [details]
Patch
Comment 12 Thiago Marcos P. Santos 2013-01-30 08:59:48 PST
Committed r141275: <http://trac.webkit.org/changeset/141275>