Closed Bug 1173451 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Extension Compatibility, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 42
Tracking Status
e10s m8+ ---
firefox41 + fixed
firefox42 --- fixed

People

(Reporter: mconley, Assigned: gkrizsanits)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
Blocks: 1177499
This is causing bug 1177499 - re-nomming.
Blocks: 1177838
Assignee: nobody → wmccloskey
Assignee: wmccloskey → gkrizsanits
Attachment #8628423 - Flags: review?(mconley)
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 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?
Attachment #8628423 - Flags: review?(mconley) → review-
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)
Attachment #8628423 - Attachment is obsolete: true
Attachment #8629336 - Flags: review?(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.
Attachment #8629336 - Flags: review?(mconley) → review+
[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.
Uploading the final version for aurora request.
Attachment #8629336 - Attachment is obsolete: true
Attachment #8630366 - Flags: review+
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.
Attachment #8630366 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/f2f753133ee9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
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?
Flags: needinfo?(mconley)
Flags: needinfo?(mconley)
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!
Flags: needinfo?(mconley)
Yes, confirmed that this is fixed.
Status: RESOLVED → VERIFIED
Flags: needinfo?(mconley)
Comment on attachment 8630366 [details] [diff] [review]
RemoteAddonsChild init should be optional. v3

Thanks Mike for the verification, approving for uplift to Aurora.
Attachment #8630366 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.