776424 - SpecialPowers.getBoolPref() shouldn't throw if trying to get a preference without value

Status: RESOLVED WONTFIX

(Testing :: Mochitest, defect)

Description

[Mounir Lamouri (:mounir)]

It should return 'null' instead. That way, callers don't have to put try/catch blocks around getPref calls and using pushPrefEnv will be allowed for preferences without a default value.
FWIW, the current code in pushPrevEnv allows to have null as |originalValue|'s value and does what has to be done in popPrefEnv.

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 2

[cmtalbert]

Comment on attachment 644818 [details] [diff] [review]
Patch

Cool. I agree with this approach.  Thanks for the patch.

Comment 3

[Mounir Lamouri (:mounir)]

BTW, this has not landed - yet - because it is breaking quite a few tests.

Comment 4

[Martijn Wargers (dead)]

Is this related to bug 766687?

Comment 5

[Mounir Lamouri (:mounir)]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #4)
> Is this related to bug 766687?

Maybe.

Comment 6

[Martijn Wargers (dead)]

Attachment: 776424.diff

I fixed the errors that I saw when going through the Harness_Sanity/ directory.
I put this patch on try server to see if other mochitests are failing because of this change: https://tbpl.mozilla.org/?tree=Try&rev=6d528d7fde7f

Comment 7

[Martijn Wargers (dead)]

Attachment: 776424.diff

Ok, content/media/test/test_can_play_type_mpeg.html was also depending on this behavior, so I fixed that in this patch.

Comment 8

[Martijn Wargers (dead)]

I'm seeing time outs in the dom/ipc/tests/test_process_error.xul mochitest-chrome on the try server with this patch.
That test was added in bug 581335. Iiuc, privateNoteIntentionalCrash() is not implemented, so apparently this code in process_error_contentscript.js is supposed to cause a crash:
var zero = new ctypes.intptr_t(8);
var badptr = ctypes.cast(zero, ctypes.PointerType(ctypes.int32_t));
var crash = badptr.contents;

It seems like with this patch, the crash isn't triggered anymore. I have no idea why that happens with this patch, it seems unrelated to that code.
Is the above code a reliable way to hit the crash?

Comment 9

[(not currently active) Ted Mielczarek]

That seems unrelated, but that code ought to reliably crash. It's simply dereferencing a near-NULL pointer using ctypes.

Comment 10

[Martijn Wargers (dead)]

Ok, pushed the latest patch to try again, to see if it still happens:
https://tbpl.mozilla.org/?tree=Try&rev=4a6d06c933a3

Comment 11

[Martijn Wargers (dead)]

That one still shows the failures, I just repulled my tree, perhaps I just had a bad tree: https://tbpl.mozilla.org/?tree=Try&rev=ed08ffd4c58d

Comment 12

[Martijn Wargers (dead)]

Ok, I think it might be happening because of:
http://mxr.mozilla.org/mozilla-central/source/content/xul/document/test/test_bug757137.xul#26
With this patch, the dom.ipc.tabs.disabled pref would have changed after the test file finishes.

There are quite some tests that seems to rely on this behavior:
http://mxr.mozilla.org/mozilla-central/search?string=specialpowers.getboolpref&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
http://mxr.mozilla.org/mozilla-central/search?string=specialpowers.getintpref
http://mxr.mozilla.org/mozilla-central/search?string=specialpowers.getcharpref

They need all to be changed. In that case, they would need to be changed to the SpecialPowers.pushPrefEnv method.

Comment 13

[Benjamin Smedberg]

I'm concerned about this approach. You're introducing an API difference between SpecialPowers.get*Pref and Preferences.get*Pref which I don't think many people will expect. We can't change the prefservice API for backward-compat reasons, and so I don't think we should make this change either. The test changes that would be required are a symptom that this is probably not a good idea.

Comment 14

[Martijn Wargers (dead)]

Ok, WONTFIX then?

Comment 15

[Martijn Wargers (dead)]

Please mark WONTFIX if you still stand by your comment 13.