Bug 108182 – REGRESSION (r141051): Broke plugin support on non-Mac WebKit2 Ports

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Thiago Marcos P. Santos]

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 ]

[Zoltan Arvai]

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

[Thiago Marcos P. Santos]

Fix is on the way.

[Thiago Marcos P. Santos]

Created attachment 185255 [details]
Patch

[Simon Hausmann]

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.

[Thiago Marcos P. Santos]

(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.

[Anders Carlsson]

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.

[Thiago Marcos P. Santos]

(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.

[Sam Weinig]

(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.

[Simon Hausmann]

(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.

[Simon Hausmann]

(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?

[Thiago Marcos P. Santos]

Created attachment 185507 [details]
Patch

[Thiago Marcos P. Santos]

Committed r141275: <http://trac.webkit.org/changeset/141275>