589431 - Crash when using profile manager [@ mozilla::imagelib::DiscardTracker::ReloadTimeout ]

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Robert Haas [:scenor]]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b5pre) Gecko/20100821 Minefield/4.0b5pre
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b5pre) Gecko/20100821 Minefield/4.0b5pre

If I start FF with the Profile-Manager, it causes a segmentation fault after choosing profileand clicking "Start Minefield"
If I then start FF again without Profile-Manager, it starts with the selected Profile.
Using the -P flag also works.

Reproducible: Always

Steps to Reproduce:
see above
Actual Results:  
see above


http://crash-stats.mozilla.com/report/index/bp-d8f259a6-c4b2-4b96-a6aa-a123f2100821

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

I think this should be enough.

Comment 2

[Daniel C]

Mozilla/5.0 (X11; Linux i686; rv:2.0b5pre) Gecko/20100821 Minefield/4.0b5pre

I only started seeing this in today's build so I'm not sure if that bug is the right one to point at.

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2d6ead22831c&tochange=5c5c3bf8dfeb

bp-0e9d9648-83f2-41e0-b94e-794732100821

bp-82a4c9b4-4415-4356-9489-39f412100821

Editing profiles.ini and changing StartWithLastProfile=0 to StartWithLastProfile=1 (with Default=1 on the correct profile) skips the Profile Manager and avoids the crash. Minefield hasn't crashed so far after that (keeping image discarding enabled).

Comment 3

[Daniel C]

I guess there's no tests that bring up the profile manager.

Simply starting with the Profile Manager and then quitting causes this crash for me (no need to even load a profile).

Comment 4

[Robert Haas [:scenor]]

(In reply to comment #3)
> I guess there's no tests that bring up the profile manager.
> 
> Simply starting with the Profile Manager and then quitting causes this crash
> for me (no need to even load a profile).

That's right for me too, just didn't test it.

Comment 5

[Robert Haas [:scenor]]

(In reply to comment #1)
> Created attachment 468027 [details] [diff] [review]
> patch
> 
> I think this should be enough.

Works for me

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 468027 [details] [diff] [review]
patch

>diff --git a/modules/libpr0n/src/DiscardTracker.cpp b/modules/libpr0n/src/DiscardTracker.cpp
>--- a/modules/libpr0n/src/DiscardTracker.cpp
>+++ b/modules/libpr0n/src/DiscardTracker.cpp
>-void
>+nsresult
> DiscardTracker::ReloadTimeout()
> {
>   nsresult rv;
> 
>   // read the timeout pref
>   PRInt32 discardTimeout;
>   nsCOMPtr<nsIPrefBranch2> branch = do_GetService(NS_PREFSERVICE_CONTRACTID);
>+  NS_ENSURE_STATE(branch);
>   rv = branch->GetIntPref(DISCARD_TIMEOUT_PREF, &discardTimeout);
> 
>-  // If we got something bogus, return
>+  // If we got something bogus, keep the old value and return.
>   if (!NS_SUCCEEDED(rv) || discardTimeout <= 0)
>-    return;
>+    return NS_OK;

I don't think it's worth causing a ruckus if something goes wrong with the prefbranch - we should just emit an NS_WARNING and return, which will make us use the default value (defined statically at the top of the file). So do that, leave DiscardTracker::ReloadTimeout() as a void method, and consequently there's no need to change DiscardTracker::Initialize().

r=bholley with that change.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

If we can't access the prefservice, something is in a strange state, like
in this case where we're actually shutting down.
IMO, we shouldn't run DiscardTracker::Initialize() then.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

..but actually that shouldn't matter much.
I'll change the patch and push it.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/b6a15d2bdb14