Closed Bug 616414 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Preferences: Backend, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Dan, do you happen to know off-hand why this might happen?
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.
Assignee: nobody → jones.chris.g
Attachment #496449 - Flags: review?(dwitte)
Attached patch Test (obsolete) — Splinter Review
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?
Attachment #496450 - Flags: feedback?(dwitte)
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).
Attachment #496450 - Flags: feedback?(dwitte) → feedback?(jduell.mcbugs)
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.
Attachment #496449 - Flags: review?(dwitte) → review+
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
Attachment #496450 - Flags: feedback?(jduell.mcbugs) → feedback+
Carrying over r+.
Attachment #496449 - Attachment is obsolete: true
Attachment #496450 - Attachment is obsolete: true
Attachment #500238 - Flags: review+
Attached patch TestSplinter Review
Multiple run_test_in_child() is not going to work without a lot more effort, so I did something simpler.
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.
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+
Depends on: 639956
You need to log in before you can comment on or make changes to this bug.