1357486 - Turn on OOP extensions by default on Windows

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P1)

Comment 1

[Andy McKay]

Marking as quantum flow p1 because this is about the ability to run webextensions in a seperate process.

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1357486: Enable OOP extensions by default on Windows.

Review commit: https://reviewboard.mozilla.org/r/155076/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155076/

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8884165 [details]
Bug 1357486: Enable OOP extensions by default on Windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155076/diff/1-2/

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8884165 [details]
Bug 1357486: Enable OOP extensions by default on Windows.

https://reviewboard.mozilla.org/r/155076/#review160144

The bug for this patch mentions Windows and OSX but the patch is Windows only?

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8884165 [details]
Bug 1357486: Enable OOP extensions by default on Windows.

https://reviewboard.mozilla.org/r/155076/#review160144

I was going to move OS-X support to a separate bug. If it works as-is after the last dependency for this bug lands, I'll turn it on. Otherwise, it's going to have to wait until after 57.

Comment 6

[Kris Maglione [:kmag]]

Attachment: Bug 1357486: Part 0 - Fix permissions tests with OOP extensions.

Review commit: https://reviewboard.mozilla.org/r/155306/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155306/

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8884165 [details]
Bug 1357486: Enable OOP extensions by default on Windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155076/diff/2-3/

Comment 8

[Kris Maglione [:kmag]]

Attachment: Bug 1357486: Part 0b - Fix inline options browser tests with OOP extensions.

Review commit: https://reviewboard.mozilla.org/r/155342/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155342/

Comment 9

[Kris Maglione [:kmag]]

Attachment: Bug 1357486: Part 0c - Propagate addonId to parent process with console messages.

Review commit: https://reviewboard.mozilla.org/r/155344/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155344/

Comment 10

[Kris Maglione [:kmag]]

Attachment: Bug 1357486: Part 0d - Propagate clonable console message args to the parent process.

Review commit: https://reviewboard.mozilla.org/r/155346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155346/

Comment 11

[Kris Maglione [:kmag]]

Attachment: Bug 1357486: Part 0e - Support legacy extensions in OOP mode.

Review commit: https://reviewboard.mozilla.org/r/155348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155348/

Comment 12

[Kris Maglione [:kmag]]

Attachment: Bug 1357486: Part 0f - Run some chrome tests in in-process mode.

Review commit: https://reviewboard.mozilla.org/r/155350/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155350/

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8884438 [details]
Bug 1357486: Part 0b - Fix inline options browser tests with OOP extensions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155342/diff/1-2/

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8884439 [details]
Bug 1357486: Part 0c - Propagate addonId to parent process with console messages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155344/diff/1-2/

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8884440 [details]
Bug 1357486: Part 0d - Propagate clonable console message args to the parent process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155346/diff/1-2/

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8884441 [details]
Bug 1357486: Part 0e - Support legacy extensions in OOP mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155348/diff/1-2/

Comment 17

[Kris Maglione [:kmag]]

Comment on attachment 8884442 [details]
Bug 1357486: Part 0f - Run some chrome tests in in-process mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155350/diff/1-2/

Comment 18

[Andrew Swan [:aswan]]

Comment on attachment 8884381 [details]
Bug 1357486: Part 0 - Fix permissions tests with OOP extensions.

https://reviewboard.mozilla.org/r/155306/#review160466

Comment 19

[Andrew Swan [:aswan]]

Comment on attachment 8884438 [details]
Bug 1357486: Part 0b - Fix inline options browser tests with OOP extensions.

https://reviewboard.mozilla.org/r/155342/#review160470

Comment 20

[Andrew Swan [:aswan]]

Comment on attachment 8884439 [details]
Bug 1357486: Part 0c - Propagate addonId to parent process with console messages.

https://reviewboard.mozilla.org/r/155344/#review160472

Comment 21

[Andrew Swan [:aswan]]

Comment on attachment 8884441 [details]
Bug 1357486: Part 0e - Support legacy extensions in OOP mode.

https://reviewboard.mozilla.org/r/155348/#review160474

::: toolkit/components/extensions/ExtensionParent.jsm:225
(Diff revision 2)
> +
> +    // If we have a remote, embedded extension, the legacy side is
> +    // running in a different process than the WebExtension side.
> +    // As a result, we need to dispatch the message to both the parent
> +    // and extension processes, and manually merge the results.
> +    if (extension.isEmbedded && extension.remote) {

nit: I think this would be easily to read if you just return `promise1` here in the common case of not having to deal with hybrid extensions and avoid putting the code below inside a block.

Comment 22

[Andrew Swan [:aswan]]

Comment on attachment 8884440 [details]
Bug 1357486: Part 0d - Propagate clonable console message args to the parent process.

https://reviewboard.mozilla.org/r/155346/#review160476

::: toolkit/components/processsingleton/ContentProcessSingleton.js:82
(Diff revision 2)
> +          try {
> +            arg = Cu.cloneInto(arg, {});
> +          } catch (e) {
> -          arg = unavailString;
> +            arg = unavailString;
> +          }

can you add a comment explaining this

Comment 23

[Andrew Swan [:aswan]]

Comment on attachment 8884442 [details]
Bug 1357486: Part 0f - Run some chrome tests in in-process mode.

https://reviewboard.mozilla.org/r/155350/#review160478

can you either mark this bug leave-open or a file a separate bug for these?  (I can take a look at them if you like)

Comment 24

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #23)
> can you either mark this bug leave-open or a file a separate bug for these? 
> (I can take a look at them if you like)

The fixes should probably be almost identical to what's needed for bug 1352239, so if you could fix it along with that, that would be great.

Comment 25

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa4a2714ae9b05ed28e76e1b61570c97ed69a81d
Bug 1357486: Part 0a - Fix permissions tests with OOP extensions. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e252b1de0d02d24067ac817d4d4de6351e4e039
Bug 1357486: Part 0b - Fix inline options browser tests with OOP extensions. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/c72d36a91e448d73a48c7f0e37175537530089b8
Bug 1357486: Part 0c - Propagate addonId to parent process with console messages. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/0159b012fa2d1135647f31689824ca6e31497a7d
Bug 1357486: Part 0d - Propagate clonable console message args to the parent process. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/44ab0cca4956dec0ea97cfce9d1503f43d3b9916
Bug 1357486: Part 0e - Support legacy extensions in OOP mode. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fba1531fbaf630674b295684a25164b64aa8fe5
Bug 1357486: Part 0f - Run some chrome tests in in-process mode. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/7124641a168ffaa20461764f2f554992dfd856e6
Bug 1357486: Enable OOP extensions by default on Windows. r=aswan

Comment 26

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb1aa3b7bdf373c86efde1face922b02eadb94fb
Bug 1357486: Follow-up: Skip optional permissions test with e10s disabled.

Comment 27

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

Backed out for frequently failing xpcshell's toolkit/mozapps/extensions/test/xpcshell/test_delay_update_webextension.js on Windows debug:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ee92fd6fac7e89be7317f36b31167b5c757ffbb
https://hg.mozilla.org/integration/mozilla-inbound/rev/0068662edb966b29283c8a1a3b30eaaf4d06d99a
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc11bba5e51f7fae68cbbeb3703ddf4b0c200085
https://hg.mozilla.org/integration/mozilla-inbound/rev/85e8fc25e21d6cb340bad5f4ca551ff92e20c2e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/9761dc56f9178fc96aefe20bfe2a764d9e56223b
https://hg.mozilla.org/integration/mozilla-inbound/rev/40566f684eddedbdfe0b77a109d645cae07a4248
https://hg.mozilla.org/integration/mozilla-inbound/rev/664ef7ecc0589aa3d716d2530a2de6a3999bd480
https://hg.mozilla.org/integration/mozilla-inbound/rev/f13478a0064a7726cdd5a257fc8d6f26d8a527ba

Follow-up push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bb1aa3b7bdf373c86efde1face922b02eadb94fb&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=success&filter-searchStr=xpcshell

Failure log xpcshell: https://treeherder.mozilla.org/logviewer.html#?job_id=112757035&repo=mozilla-inbound

Is it caused by this?
01:12:50     INFO -  PID 9556 | 1499501568333	addons.xpi	ERROR	Failed to remove file c:\\users\\cltbld\\appdata\\local\\temp\\xpc-profile-qex9qa\\extensions\\trash\\[e-mail]: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.remove]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm :: recursiveRemove :: line 1054"  data: no] Stack trace: recursiveRemove()@resource://gre/modules/addons/XPIProvider.jsm:1054 < recursiveRemove()@resource://gre/modules/addons/XPIProvider.jsm:1068 < uninstallAddon()@resource://gre/modules/addons/XPIProvider.jsm:6254 < uninstallAddon()@resource://gre/modules/addons/XPIProvider.jsm:4658 < uninstall()@resource://gre/modules/addons/XPIProvider.jsm:5515 < unload()@resource://testing-common/ExtensionXPCShellUtils.jsm:303
01:12:50     INFO -  PID 9556 | 1499501568340	addons.xpi	WARN	Failed to remove trash directory when uninstalling [e-mail]: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.remove]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm :: recursiveRemove :: line 1054"  data: no] Stack trace: recursiveRemove()@resource://gre/modules/addons/XPIProvider.jsm:1054 < recursiveRemove()@resource://gre/modules/addons/XPIProvider.jsm:1068 < uninstallAddon()@resource://gre/modules/addons/XPIProvider.jsm:6254 < uninstallAddon()@resource://gre/modules/addons/XPIProvider.jsm:4658 < uninstall()@resource://gre/modules/addons/XPIProvider.jsm:5515 < unload()@resource://testing-common/ExtensionXPCShellUtils.jsm:303

Comment 28

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

This causes also these devtools failures: https://treeherder.mozilla.org/logviewer.html#?job_id=112757307&repo=mozilla-inbound

Comment 29

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/07cb3b166564bcf859976c5941761a194b97b695
Bug 1357486: Part 0a - Fix permissions tests with OOP extensions. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa1b70cb93415506b63604d44163bf697784a068
Bug 1357486: Part 0b - Fix inline options browser tests with OOP extensions. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/df8b19189f4c2ae7f2e91d07ca473c471310618b
Bug 1357486: Part 0c - Propagate addonId to parent process with console messages. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/312a55aa2bcc69882ab3388a15390dce6c0c6530
Bug 1357486: Part 0d - Propagate clonable console message args to the parent process. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cf94b3218de0e695b970cf204569bcc94b95a1a
Bug 1357486: Part 0e - Support legacy extensions in OOP mode. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bf31f8f11ef551180d5b7ae358217f049cddc68
Bug 1357486: Part 0f - Run some chrome tests in in-process mode. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/d308c26c9b213fb957c8a5192bd425674f4cbd0d
Bug 1357486: Part 0g - Run remote debugger host browser in same TabGroup as extension pages. r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/a625a2e9b3333a8e76982ea65f077cfded6ac224
Bug 1357486: Enable OOP extensions by default on Windows. r=aswan

Comment 30

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6664e29e12ac9e96265d8425a6ff86dc16a16c06
Bug 1357486: Follow-up - Run flaky update tests in in-process mode on win32 debug.

Comment 31

[Gary [:streetwolf52]]

It appears that this patch has partially broken at least one WE add-on I use.  It is called Zoom Page WE. The dropdown lists on the main menu to change font size and zoom level do not display with this patch.  I have notified the developer of this problem.

Comment 32

[Gary [:streetwolf52]]

UA:  Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0

I set extensions.webextensions.remote to false as a workaround.

Comment 33

[Kris Maglione [:kmag]]

(In reply to Gary [:streetwolf] from comment #31)
> It appears that this patch has partially broken at least one WE add-on I
> use.  It is called Zoom Page WE. The dropdown lists on the main menu to
> change font size and zoom level do not display with this patch.  I have
> notified the developer of this problem.

This patch hasn't merged to central or appeared in any nightly yet. Where are you seeing this breakage?

Comment 34

[Gary [:streetwolf52]]

I'm on inbound.

Comment 35

[Gary [:streetwolf52]]

Attachment: Screenshot with patch enabled

When Clicking on the items circled in red a drop down list of font sizes and zoom level percentages should appear.  With patch no list.  With pref set to false I get the list.

Comment 36

[Gary [:streetwolf52]]

Attachment: Screenshot with patch disabled

Pref set to false.  Zoom level also has a drop down list.

Comment 37

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23c63ef286cc35e1e31aa6a9afcaf00d227bf9d6
Bug 1357486: Follow-up: Wait for extension shutdown before starting storage shutdown. r=rhelmer

Comment 38

[Kris Maglione [:kmag]]

(In reply to Gary [:streetwolf] from comment #35)
> Created attachment 8884659 [details]
> Screenshot with patch enabled
> 
> When Clicking on the items circled in red a drop down list of font sizes and
> zoom level percentages should appear.  With patch no list.  With pref set to
> false I get the list.

Ah, I guess we fail to paint select boxes for OOP popups. Can you file a bug?

Comment 39

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7c72fa8ccd849c4cc09ca1994b6e22b5ea4bc17
Bug 1357486: Follow-up: Don't propagate rejections to AsyncShutdown.

Comment 40

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bafb677d3d3c40cb824ef591a5bd096291ee373a
Bug 1357486: Follow-up: Fix error on shutdown after incomplete startup.

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/07cb3b166564
https://hg.mozilla.org/mozilla-central/rev/fa1b70cb9341
https://hg.mozilla.org/mozilla-central/rev/df8b19189f4c
https://hg.mozilla.org/mozilla-central/rev/312a55aa2bcc
https://hg.mozilla.org/mozilla-central/rev/6cf94b3218de
https://hg.mozilla.org/mozilla-central/rev/1bf31f8f11ef
https://hg.mozilla.org/mozilla-central/rev/d308c26c9b21
https://hg.mozilla.org/mozilla-central/rev/a625a2e9b333
https://hg.mozilla.org/mozilla-central/rev/6664e29e12ac
https://hg.mozilla.org/mozilla-central/rev/23c63ef286cc
https://hg.mozilla.org/mozilla-central/rev/b7c72fa8ccd8
https://hg.mozilla.org/mozilla-central/rev/bafb677d3d3c

Comment 42

[Joel Maher ( :jmaher ) (UTC -8)]

some perf improvements:
== Change summary for alert #7870 (as of July 12 2017 17:06 UTC) ==

Improvements:

 12%  tp5o_webext responsiveness windows10-64 pgo e10s     4.16 -> 3.64
  6%  tp5o responsiveness windows10-64 opt e10s            4.06 -> 3.79
  3%  tp5o_webext responsiveness windows10-64 opt e10s     4.26 -> 4.12

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7870