772310 - Marionette hits getBoolPref error on startup

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Jesse Ruderman]

When starting a tbox debug build of Firefox on Mac (with a fresh profile), I get this error:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: resource:///components/marionettecomponent.js :: mc_observe :: line 33"  data: no]
************************************************************

> if (Services.prefs.getBoolPref('marionette.defaultPrefs.enabled')) {

Comment 1

[Jonathan Griffin (:jgriffin)]

Attachment: patch v0.1

See if the pref exists before we try to read from it.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 641247 [details] [diff] [review]
patch v0.1

>+const MARIONETTE_ENABLED_PREF = 'marionette.defaultPrefs.enabled';
[..]
>-        if (Services.prefs.getBoolPref('marionette.defaultPrefs.enabled')) {
>+        if (Services.prefs.prefHasUserValue(MARIONETTE_ENABLED_PREF) && 
>+            Services.prefs.getBoolPref(MARIONETTE_ENABLED_PREF)) {

Why are we making use of 'defaultPref'? We should use 'marionette.enabled' and set it as default in firefox.js. Whenever it is changed it will automatically has an user value. That way getBoolPref should never fail.

Comment 3

[Jonathan Griffin (:jgriffin)]

(In reply to Henrik Skupin (:whimboo) from comment #2)
> Comment on attachment 641247 [details] [diff] [review]
> patch v0.1
> 
> >+const MARIONETTE_ENABLED_PREF = 'marionette.defaultPrefs.enabled';
> [..]
> >-        if (Services.prefs.getBoolPref('marionette.defaultPrefs.enabled')) {
> >+        if (Services.prefs.prefHasUserValue(MARIONETTE_ENABLED_PREF) && 
> >+            Services.prefs.getBoolPref(MARIONETTE_ENABLED_PREF)) {
> 
> Why are we making use of 'defaultPref'? We should use 'marionette.enabled'
> and set it as default in firefox.js. Whenever it is changed it will
> automatically has an user value. That way getBoolPref should never fail.

It probably should be just 'marionette.enabled', but that's a separate issue.

Marionette is not included in release builds of Firefox, so I don't believe it makes sense to add a pref to firefox.js, that will never be accessed.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Jonathan Griffin (:jgriffin) from comment #3)
> It probably should be just 'marionette.enabled', but that's a separate issue.

Has that already been filed? If not I can happily do so.

> Marionette is not included in release builds of Firefox, so I don't believe
> it makes sense to add a pref to firefox.js, that will never be accessed.

I'm sure you can '#ifdef' that for whatever builds except releases. Also here I'm happy to file a bug on it if necessary.

Comment 5

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> (In reply to Jonathan Griffin (:jgriffin) from comment #3)
> > It probably should be just 'marionette.enabled', but that's a separate issue.
> 
> Has that already been filed? If not I can happily do so.

Sure, I don't think we need defaultPrefs anymore, that was from back when we were trying out different security methods.
> 
> > Marionette is not included in release builds of Firefox, so I don't believe
> > it makes sense to add a pref to firefox.js, that will never be accessed.
> 
> I'm sure you can '#ifdef' that for whatever builds except releases. Also
> here I'm happy to file a bug on it if necessary.

I don't think we want to dot hat since we don't want to set this value by default, it's something only used by test profiles, and in b2g.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Malini Das [:mdas] from comment #5)
> > I'm sure you can '#ifdef' that for whatever builds except releases. Also
> > here I'm happy to file a bug on it if necessary.
> 
> I don't think we want to dot hat since we don't want to set this value by
> default, it's something only used by test profiles, and in b2g.

Oh I see. That makes sense then.

Comment 7

[Jonathan Griffin (:jgriffin)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/70a11a81d2c5

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/70a11a81d2c5