616414 - "Conditional jump or move depends on uninitialised value(s)" at IPC::ParamTraits<PrefTuple>::Write (caused by ContentParent::Observe)

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Problem is at

    if (!strcmp(aTopic, "nsPref:changed")) {
        // We know prefs are ASCII here.
        NS_LossyConvertUTF16toASCII strData(aData);

        PrefTuple pref;
        nsCOMPtr<nsIPrefServiceInternal> prefService =
          do_GetService("@mozilla.org/preferences-service;1");

        prefService->MirrorPreference(strData, &pref);

where the last stmt above can fail at

NS_IMETHODIMP nsPrefService::MirrorPreference(const nsACString& aPrefName,
                                              PrefTuple *aPref)
{
  PrefHashEntry *pref = pref_HashTableLookup(nsDependentCString(aPrefName).get());

  if (!pref)
    return NS_ERROR_NOT_AVAILABLE;

if |pref| doesn't exist.  The only reasonable hypothesis I can think of for that failure mode is that |aPrefName| has been deleted, in which case ContentParent needs to send a message indicating that instead of uninitialized garbage.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Dan, do you happen to know off-hand why this might happen?

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Looks like it's a pref being cleared.

(gdb) bt
#0  mozilla::dom::ContentParent::Observe (this=0x7fffe01290d0, aSubject=0x7748c0, aTopic=0x7ffff643d50e "nsPref:changed", aData=0x7fffffff6bb0) at /home/cjones/mozilla/mozilla-central/dom/ipc/ContentParent.cpp:366
#1  0x00007ffff45f1475 in nsPrefBranch::NotifyObserver (newpref=0xba6820 "gfx.color_management.force_srgb", data=0x7fffe012d2b0) at /home/cjones/mozilla/mozilla-central/modules/libpref/src/nsPrefBranch.cpp:731
#2  0x00007ffff45fab85 in pref_DoCallback (changed_pref=0xba6820 "gfx.color_management.force_srgb") at /home/cjones/mozilla/mozilla-central/modules/libpref/src/prefapi.cpp:970
#3  0x00007ffff45fa09b in PREF_ClearUserPref (pref_name=0xba6820 "gfx.color_management.force_srgb") at /home/cjones/mozilla/mozilla-central/modules/libpref/src/prefapi.cpp:634
#4  0x00007ffff45f07c0 in nsPrefBranch::ClearUserPref (this=0x7748c0, aPrefName=0xba6820 "gfx.color_management.force_srgb") at /home/cjones/mozilla/mozilla-central/modules/libpref/src/nsPrefBranch.cpp:491
#5  0x00007ffff45f7cba in nsPrefService::ClearUserPref (this=0x72ef40, aPrefName=0xba6820 "gfx.color_management.force_srgb") at /home/cjones/mozilla/mozilla-central/modules/libpref/src/nsPrefService.h:62

(gdb) printf "%s", PrintJSStack()
0 OnRefTestUnload() ["chrome://reftest/content/reftest.js":329]
    prefs = [xpconnect wrapped nsIPrefBranch2 @ 0x10555f0 (native @ 0x72ef40)]
    this = [object ChromeWindow @ 0xe67850 (native @ 0xa4b348)]
1 onunload(event = [object Event @ 0xcd90c0 (native @ 0xedc050)]) ["chrome://reftest/content/reftest.xul":1]
    this = [object ChromeWindow @ 0xe67850 (native @ 0xa4b348)]

function OnRefTestUnload()
{
    /* Clear the sRGB forcing pref to leave the profile as we found it. */
    var prefs = Components.classes["@mozilla.org/preferences-service;1"].
                getService(Components.interfaces.nsIPrefBranch2);
    prefs.clearUserPref("gfx.color_management.force_srgb");

Me trying to post this comment was apparently the first hint of The Great Bugzilla Outage of '10, and during it I just about finished up a fix for clearing.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Notify content processes when a pref has been cleared

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Test

Before the patch, we get

child: TEST-INFO | (xpcshell/head.js) | test 1 pending\nchild: TEST-PASS | /home/cjones/mozilla/ff-dbg/_tests/xpcshell/modules/libpref/test/unit_ipc/test_observed_prefs.js | [run_test : 12] true == true\nchild: TEST-PASS | /home/cjones/mozilla/ff-dbg/_tests/xpcshell/modules/libpref/test/unit_ipc/test_observed_prefs.js | [run_test : 13] 23 == 23\nchild: TEST-PASS | /home/cjones/mozilla/ff-dbg/_tests/xpcshell/modules/libpref/test/unit_ipc/test_observed_prefs.js | [run_test : 14] hey == hey\nchild: TEST-INFO | (xpcshell/head.js) | test 1 finished\nchild: TEST-INFO | (xpcshell/head.js) | exiting test\nchild: TEST-PASS | (xpcshell/head.js) | 3 (+ 0) check(s) passed\n###!!! ASSERTION: Unknown type: 'Not Reached', file /home/cjones/mozilla/mozilla-central/modules/libpref/src/prefapi.cpp, line 328
UNKNOWN [/home/cjones/mozilla/ff-dbg/dist/bin/libxul.so +0x0093E438]
UNKNOWN [/home/cjones/mozilla/ff-dbg/dist/bin/libxul.so +0x0093A7AB]
parent: TEST-INFO | (xpcshell/head.js) | test 2 finished
UNKNOWN [/home/cjones/mozilla/ff-dbg/dist/bin/libxul.so +0x01D203C4]
UNKNOWN [/home/cjones/mozilla/ff-dbg/dist/bin/libxul.so +0x01E8C1ED]
&c

(Ugh, check-one reverted back to not unbuffering.  Ah well.)

After

child: TEST-INFO | (xpcshell/head.js) | test 1 pending\nchild: TEST-PASS | /home/cjones/mozilla/ff-dbg/_tests/xpcshell/modules/libpref/test/unit_ipc/test_observed_prefs.js | [run_test : 12] true == true\nchild: TEST-PASS | /home/cjones/mozilla/ff-dbg/_tests/xpcshell/modules/libpref/test/unit_ipc/test_observed_prefs.js | [run_test : 13] 23 == 23\nchild: TEST-PASS | /home/cjones/mozilla/ff-dbg/_tests/xpcshell/modules/libpref/test/unit_ipc/test_observed_prefs.js | [run_test : 14] hey == hey\nchild: TEST-INFO | (xpcshell/head.js) | test 1 finished\nchild: TEST-INFO | (xpcshell/head.js) | exiting test\nchild: TEST-PASS | (xpcshell/head.js) | 3 (+ 0) check(s) passed\ntypein:TypeError: redeclaration of const _TEST_FILE
parent: TEST-INFO | (xpcshell/head.js) | test 2 finished

Oops, I didn't notice "TypeError: redeclaration of const _TEST_FILE" before pasting here.  Any idea what that might be?

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 496450 [details] [diff] [review]
Test

Hmm, I wonder if run_test_in_child() isn't set up to run multiple child scripts.  That's OK, I guess I could modify the test (would be happy with suggestions).

Comment 6

[[e-mail]]

Comment on attachment 496449 [details] [diff] [review]
Notify content processes when a pref has been cleared

>diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp

>+        nsresult rv = prefService->MirrorPreference(strData, &pref);
>+        if (NS_SUCCEEDED(rv)) {
>+            // Pref was created, or previously existed and its value
>+            // changed.
>+            if (!SendPreferenceUpdate(pref))
>+                return NS_ERROR_NOT_AVAILABLE;
>+        } else {
>+            // Pref wasn't found.  It was probably removed.
>+            if (!SendClearUserPreference(strData))
>+                return NS_ERROR_NOT_AVAILABLE;
>+        }

I'd rather be a bit more explicit here. Instead of trying MirrorPreference() and seeing if it works or not, how about:

if prefService->PrefHasUserValue(...) then
  prefService->MirrorPreference(...) and assert success
  SendPreferenceUpdate(...)
else
  SendClearUserPreference(...)

>diff --git a/modules/libpref/public/nsIPrefService.idl b/modules/libpref/public/nsIPrefService.idl

>+  [noscript] void clearUserPreference(in ACString aPrefName);

Sigh. This is tragically similar in name to nsIPrefBranch.clearUserPref(). I don't think we can get around that, but maybe we could give it a slightly less similar name? clearContentPref perhaps?

What would be really awesome is if we provided a global function to get the nsPrefService* singleton itself, like we do in cookies, and then these internal methods were only available there. ;)

Anyway, r=dwitte with the name change.

Comment 7

[Jason Duell]

Comment on attachment 496450 [details] [diff] [review]
Test

>     run_test_in_child("test_observed_prefs.js");
>+
>+    pb.clearUserPref("Test.IPC.bool.new");
>+    run_test_in_child("test_cleared_prefs.js");

run_test_in_child takes an optional callback function (documented in head.js!) that runs in the parent when the code sent to the child has completed.  So I'm guessing you may want to change your serial calls to run_test_in_child to instead have the first call take a callback that calls the second run_test_in_child.   

(But no one AFAIK is actually using multiple run_test_in_child, so while this ought to work, it might not)

Note that since run_test_in_child returns "immediately" (possibly before code is run in child) if test_observed_prefs.js should run/complete before clearUserPref, you'll want to move the latter to the callback as well.

Assuming you can get that to work (how is it failing now?), +r

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Notify content processes when a pref has been cleared

Carrying over r+.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Test

Multiple run_test_in_child() is not going to work without a lot more effort, so I did something simpler.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oops forgot to nom this.

Needs to block because there's a loss-of-functionality bug here, not propagating user-pref clears to the content process, and also because in the current code this bug results in garbage values being used in the content process.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/549be0618b0a
http://hg.mozilla.org/mozilla-central/rev/79c44d25229d