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, gustavo, 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+

Thiago Marcos P. Santos
Reported 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 ]
Attachments
Patch (14.81 KB, patch)
2013-01-29 09:49 PST, Thiago Marcos P. Santos 		no flags
DetailsFormatted DiffDiff
Patch (13.32 KB, patch)
2013-01-30 08:01 PST, Thiago Marcos P. Santos 		sam: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Zoltan Arvai
Comment 1 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
Thiago Marcos P. Santos
Comment 2 2013-01-29 08:49:47 PST
Fix is on the way.
Thiago Marcos P. Santos
Comment 3 2013-01-29 09:49:39 PST
Created attachment 185255 [details] Patch
Simon Hausmann
Comment 4 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.
Thiago Marcos P. Santos
Comment 5 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.
Anders Carlsson
Comment 6 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.
Thiago Marcos P. Santos
Comment 7 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.
Sam Weinig
Comment 8 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.
Simon Hausmann
Comment 9 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.
Simon Hausmann
Comment 10 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?
Thiago Marcos P. Santos
Comment 11 2013-01-30 08:01:03 PST
Created attachment 185507 [details] Patch
Thiago Marcos P. Santos
Comment 12 2013-01-30 08:59:48 PST
Committed r141275: <http://trac.webkit.org/changeset/141275>
Note You need to log in before you can comment on or make changes to this bug.