1173451 - Lots of JavaScript error: resource://gre/modules/RemoteAddonsChild.jsm, line 27: TypeError: invalid 'in' operand dict in safe mode with e10s enabled

Status: VERIFIED FIXED

(Firefox :: Extension Compatibility, defect)

Description

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

STR:

0) Make sure the Browser Content Toolbox is enabled by following these instructions: http://screencast.com/t/fBpGhGgqwQV
1) With the patch from bug 1172491 applied, restart in safe mode (Help -> Restart with add-ons disabled)
2) Visit some webpages while monitoring console of the Browser Content Toolbox.

ER:

No error output from the add-on shims.

AR:

Lots of JavaScript error: resource://gre/modules/RemoteAddonsChild.jsm, line 27: TypeError: invalid 'in' operand dict


It looks like in safe-mode (or maybe just with no add-ons enabled?) NotificationTracker._paths is undefined, and is being passed into setDefault as the dict.

Comment 1

[Gabor Krizsanits (INACTIVE)]

I think the reason for that is that we only load RemoteAddonsParent when a multiprocess shim is needed (which is not the case when add-ons are turned off) but we load RemoteAddonsChild unconditionally and init it as well. I think we just need to check it somehow in RemoteAddinsChild.init if multiprocess shims are being used or not, or alternatively just load and init multiprocess shims by default too.

Comment 2

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

This is causing bug 1177499 - re-nomming.

Comment 3

[Gabor Krizsanits (INACTIVE)]

Attachment: RemoteAddonsChild init should be optional. v1

Comment 4

[Gabor Krizsanits (INACTIVE)]

RemoteAddonsParent is inited only when it's needed, in safe mode that is not the
case. So on the child side we check if parent side is inited with a message
and if it's not there, we just stop the init process. Since useSyncWebProgress
is used in browser-child.js too, I just made sure it does not throw (in findPaths)

Comment 5

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

Comment on attachment 8628423 [details] [diff] [review]
RemoteAddonsChild init should be optional. v1

Review of attachment 8628423 [details] [diff] [review]:
-----------------------------------------------------------------

This appears to work, but this adds to our pattern of sending up sync messages to the parent when creating a new TabChild. :(

Would it be possible to use the initialProcessData infrastructure that billm added in bug 1162838 to tell the content process that the parent is running the shims, or does the parent not init the shimming until after it starts up the first content process?

Comment 6

[Gabor Krizsanits (INACTIVE)]

Attachment: RemoteAddonsChild init should be optional. v2

You are right, initialProcessData seems like a better approach.

RemoteAddonsParent is inited only when it's needed, in safe mode that is not the
case. So on parent side after the init we set a flag on initialProcessData and
use that from the child side to decide if init is needed. Since useSyncWebProgress
is used in browser-child.js too, I just made sure it does not throw (in findPaths)

Comment 7

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

Comment on attachment 8629336 [details] [diff] [review]
RemoteAddonsChild init should be optional. v2

Review of attachment 8629336 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good! Glad the initialProcessData thing works.

Also, we might want to get this uplifted to Aurora.

::: toolkit/components/addoncompat/RemoteAddonsChild.jsm
@@ +82,5 @@
>      }
>    },
>  
>    findPaths: function(prefix) {
> +    if (!this._paths)

This rest of this file seems to put braces around one-liners like this, so we might as well be consistent.

@@ +531,2 @@
>      if (!this._ready) {
> +      let cpmm = Cc["@mozilla.org/childprocessmessagemanager;1"]

I think you can use Services.cpmm.initialProcessData

::: toolkit/components/addoncompat/RemoteAddonsParent.jsm
@@ +959,5 @@
>    init: function() {
>      let mm = Cc["@mozilla.org/globalmessagemanager;1"].getService(Ci.nsIMessageListenerManager);
>      mm.addMessageListener("Addons:RegisterGlobal", this);
>  
> +    Services.ppmm.initialProcessData.remoteAddonsParentInited = true;

Tiny nit - I'd prefer this to be remoteAddonsParentInitted, with 2 t's.

Comment 8

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

[Tracking Requested - why for this release]:

Certain functions are broken with e10s enabled in safe mode without this patch. That's not a great state for safe mode to be in.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2f753133ee9

Comment 10

[Gabor Krizsanits (INACTIVE)]

Attachment: RemoteAddonsChild init should be optional. v3

Uploading the final version for aurora request.

Comment 11

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8630366 [details] [diff] [review]
RemoteAddonsChild init should be optional. v3

Approval Request Comment
[Feature/regressing bug #]: Bug 1172491
[User impact if declined]: Various breakage in safe mode like bug 1177499 and bug 1177838. It's already on aurora and it's quite bad since the safe mode is often used to ask contributors to test bugs without add-on.
[Describe test coverage new/current, TreeHerder]: No extra tests were added but the many errors from the console are gone, and the existing tests are working.
[Risks and why]: I don't see any special risk here.
[String/UUID change made/needed]: None.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 13

[Kate Glazko]

Tracking because it seems to affect 41 (see comment 8). Please keep us informed about uplifting to 41 Aurora. Also, Mike Conley, can you set the 41 status flags?

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Mike, could you please verify that this bug is fixed on a latest nightly build? I would like to approve for uplift to Aurora but waiting for help on verification of fix. Thanks!

Comment 15

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

Yes, confirmed that this is fixed.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8630366 [details] [diff] [review]
RemoteAddonsChild init should be optional. v3

Thanks Mike for the verification, approving for uplift to Aurora.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cea079085434