1285885 - [e10s-multi] Plugin activation should affect all content processes.

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P3)

Description

[Gabor Krizsanits (INACTIVE)]

I have not got the time to fully understand this failure, but when I run tests with multiple content processes I get this failure:

browser/base/content/test/plugins/browser_CTP_crashreporting.js | Plugin should be activated from previous test -

And it looks like plugin activation is not multiple content process ready. This might be only a bug in the testing helpers as no one reported this problem so far...

Comment 1

[Gabor Krizsanits (INACTIVE)]

I think in a multiple content process setup we should send the ActivatePlugin message to all content processes here, with the pluginInfo:

http://searchfox.org/mozilla-central/source/browser/base/content/browser-plugins.js#206

Then cache the pluginInfo parent side, and then for each new content process spawned, when their PluginContent.jsm is loaded, we should check if the plugins should be activated or not, and for that fetch the cached pluginInfo entries.

Georg, I'm not familiar with plugin activation invariants, does this sound right to you?

Comment 2

[Georg Fritzsche [:gfritzsche]]

mconley rewrote this for e10s and might know.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Just swapping this stuff in now...

We store the site-specific allowance permission in the Permission Manager via this bit from the click-to-play notification dialog:

http://searchfox.org/mozilla-central/rev/bfcc10319e4e3ce78367fa9bba9316f7eb5248b6/browser/base/content/browser-plugins.js#142

and it looks like permissions are propagated to content processes already:

http://searchfox.org/mozilla-central/rev/bfcc10319e4e3ce78367fa9bba9316f7eb5248b6/extensions/cookie/nsPermissionManager.cpp#1590

So is that not working properly? Is the permission not being set and propagated somehow?

Because ideally what'd happen is that once the parent broadcasts the activate plugin message to the child and the permission is set, if a new tab is opened and a new content process spun up, it'd share the same permissions set, and know that the site is okay to run the plugin, without having to re-query the parent.

Or am I misunderstanding something?

Comment 4

[Gabor Krizsanits (INACTIVE)]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #3)
> So is that not working properly? Is the permission not being set and
> propagated somehow?
> 
> Because ideally what'd happen is that once the parent broadcasts the
> activate plugin message to the child and the permission is set, if a new tab
> is opened and a new content process spun up, it'd share the same permissions
> set, and know that the site is okay to run the plugin, without having to
> re-query the parent.

Yeah, that part seems to be implemented correctly indeed. After a crazy amount of
printf-ing I found a hack that causes this failure (from bug 1068635). It's some
lovely b2g related hack. http://searchfox.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#1680

      // When we do the initial addition of the permissions we don't want to
      // inherit session specific permissions from other tabs or apps
      // so we ignore them and set the permission to PROMPT_ACTION if it was
      // previously allowed or denied by the user.
      if (aIgnoreSessionPermissions &&
          aExpireType == nsIPermissionManager::EXPIRE_SESSION) {
        aPermission = nsIPermissionManager::PROMPT_ACTION;
        aExpireType = nsIPermissionManager::EXPIRE_NEVER;
      }

I don't think this makes any sense for non-b2g builds. In fact I think this is incorrect.

Andrea, I think you reviewed that patch, do you think we could ifdef this out somehow so it only affects b2g builds? If so, do you know what flag I should use for that? Or should I just remove this (not sure if we care any longer about the original problem this hack is trying to solve, and it is a bug for non-b2g)

Comment 5

[Andrea Marchesini [:baku]]

Wondering if we can get rid of this b2g code completely.

The reason why we had this is because when a process A (it means the app A at that time for b2g) was changing a permission, we didn't want process B to inherit the decision of process A. So, if the permission was marked as EXPIRE_SESSION, we marked it as PROMPT + EXPIRE_NEVER. I think this was the reason why we had this code.

I can disable all of this if the current build is not b2g.

Comment 6

[Gabor Krizsanits (INACTIVE)]

Attachment: Turn off PROMPT_ACTION permission hack for non-b2g builds. v1

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a22eca82a2d
Turn off PROMPT_ACTION permission hack for non-b2g builds. r=amarchesini

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for xpcshell failure in extensions/cookie/test/unit_ipc/test_parent.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d5fd7ee95cf

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2a22eca82a2d4dd6562acd700d588834df6780ce
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=33007296&repo=mozilla-inbound

11:37:24     INFO -  TEST-START | extensions/cookie/test/unit_ipc/test_parent.js
11:42:24  WARNING -  TEST-UNEXPECTED-TIMEOUT | extensions/cookie/test/unit_ipc/test_parent.js | Test timed out
11:42:24     INFO -  TEST-INFO took 300014ms
11:42:24     INFO -  >>>>>>>
11:42:24     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
11:42:24     INFO -  (xpcshell/head.js) | test pending (2)
11:42:24     INFO -  (xpcshell/head.js) | test run in child pending (3)
11:42:24     INFO -  (xpcshell/head.js) | test MAIN run_test finished (3)
11:42:24     INFO -  running event loop
11:42:24     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: assignment to undeclared variable _XPCSHELL_PROCESS" {file: "/home/worker/workspace/build/tests/xpcshell/head.js" line: 1259}]"
11:42:24     INFO -  "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
11:42:25     INFO -  "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
11:42:25     INFO -  PROCESS | 9511 | (xpcshell:9511): Gdk-CRITICAL **: gdk_screen_get_root_window: assertion `GDK_IS_SCREEN (screen)' failed
11:42:25     INFO -  PROCESS | 9511 | (xpcshell:9511): Gtk-CRITICAL **: gtk_settings_get_for_screen: assertion `GDK_IS_SCREEN (screen)' failed
11:42:25     INFO -  PROCESS | 9511 | (xpcshell:9511): Gdk-CRITICAL **: gdk_screen_get_resolution: assertion `GDK_IS_SCREEN (screen)' failed
11:42:25     INFO -  PROCESS | 9511 | (xpcshell:9511): Gdk-CRITICAL **: gdk_screen_get_root_window: assertion `GDK_IS_SCREEN (screen)' failed
11:42:25     INFO -  CHILD-TEST-STARTED
11:42:25     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
11:42:25     INFO -  TEST-PASS | extensions/cookie/test/unit_ipc/test_parent.js | run_test - [run_test : 52] 1 == 1
11:42:25  WARNING -  TEST-UNEXPECTED-FAIL | extensions/cookie/test/unit_ipc/test_parent.js | run_test - [run_test : 53] 2 == 3
11:42:25     INFO -  /home/worker/workspace/build/tests/xpcshell/tests/extensions/cookie/test/unit_ipc/test_child.js:run_test:53
11:42:25     INFO -  /home/worker/workspace/build/tests/xpcshell/head.js:_execute_test:534
11:42:25     INFO -  typein:null:0
11:42:25     INFO -  exiting test
11:42:25     INFO -  CHILD-TEST-COMPLETED
11:42:25     INFO -  (xpcshell/head.js) | test finished (2)
11:42:25     INFO -  <<<<<<<

Comment 9

[Gabor Krizsanits (INACTIVE)]

There was another patch under bug 1068635 that changed a test to support the hack I removed. I change back the test. On my try run I forgot to run xpcshell tests, that's why I haven't noticed it.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d28ba5a83562
Turn off PROMPT_ACTION permission hack for non-b2g builds. r=amarchesini

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d28ba5a83562