478398 - Don't hard code get_discard_timer_ms, use a pref instead.

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[deadfones]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090211 Firefox/3.2a1pre (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090211 Firefox/3.2a1pre (.NET CLR 3.5.30729)

A pref should be added for get_discard_timer_ms, as mentioned its comment.  Those with ram to spare will appreciate the speed increase with image-heavy pages.

mozilla-central\modules\libpr0n\src\imgContainer.cpp

static int
get_discard_timer_ms (void)
{
  /* FIXME: don't hardcode this */
  return 15000; /* 15 seconds */
}

Reproducible: Always

Steps to Reproduce:
NA
Actual Results:  
NA

Expected Results:  
NA

Comment 1

[deadfones]

Attachment: Possible patch

Comment 2

[deadfones]

Comment on attachment 362374 [details] [diff] [review]
Possible patch

diff --git a/modules/libpr0n/src/imgContainer.cpp b/modules/libpr0n/src/imgContainer.cpp
--- a/modules/libpr0n/src/imgContainer.cpp
+++ b/modules/libpr0n/src/imgContainer.cpp
@@ -58,6 +58,10 @@
 #include "prenv.h"

 #include "gfxContext.h"
+
+#include "nsIPrefBranch.h"
+#include "nsIPrefService.h"
+#include "nsIServiceManager.h"

 /* Accounting for compressed data */
 #if defined(PR_LOGGING)
@@ -1208,8 +1212,18 @@ static int
 static int
 get_discard_timer_ms (void)
 {
-  /* FIXME: don't hardcode this */
-  return 15000; /* 15 seconds */
+  nsresult rv;
+
+  nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
+  if (NS_FAILED(rv))
+    return rv;
+
+  PRInt32 discardtimer;
+  rv = prefs->GetIntPref("image.cache.discard_timer", &discardtimer);
+  if (NS_SUCCEEDED(rv))
+    return discardtimer;
+  else
+    return 15000; /* 15 seconds */
 }

 void
diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js
--- a/modules/libpref/src/init/all.js
+++ b/modules/libpref/src/init/all.js
@@ -2686,6 +2686,8 @@ pref("image.cache.size", 5242880);
 // A weight, from 0-1000, to place on time when comparing to size.
 // Size is given a weight of 1000 - timeweight.
 pref("image.cache.timeweight", 500);
+// Timer, in ms, to discard decoded images from cache
+pref("image.cache.discard_timer", 15000);

 #ifdef XP_WIN
 #ifndef WINCE

Comment 3

[deadfones]

Attachment: diff file (maybe) this time

It's my first time, please be gentle.

Comment 4

[deadfones]

Attachment: Patch with 8 lines

Comment 5

[Nochum Sossonko [:Natch]]

Comment on attachment 362380 [details] [diff] [review]
Patch with 8 lines

You have to request review for your patch, generally you can find reviewers at http://www.mozilla.org/owners.html

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 362380 [details] [diff] [review]
Patch with 8 lines

It's great to see a new contributor join the developer community! Let me know if I can do anything to help you out.

I have only one major problem with this patch: in failure conditions, it's going to return some nonsense value (like 0x80000000) to the timer. I'd rather it return the 15 second hard-coded default in all those cases. You can do that with a series of cascading if (NS_SUCCEEDED) checks, and then just return 15s as a fallback.

r=joe with that change.

(I also have a minor worry: we call this function on every reset of the timer. Is querying for the pref service, and then querying for the pref, going to be expensive?)

Comment 7

[Philip Chee]

> You have to request review for your patch, generally you can find reviewers at

I don't think you can access the review flags unless you have editbugs and new users generally don't even have canconfirm.

> r=joe with that change.

Does ImageLib require sr?

Comment 8

[Philip Chee]

> (I also have a minor worry: we call this function on every reset of the timer.
> Is querying for the pref service, and then querying for the pref, going to be
> expensive?)

We could do use a pref observer e.g.
<http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpHandler.cpp#227

Comment 9

[deadfones]

I'll do a different patch with a pref observer modeling nsHttpHandler.cpp#227.  Will that also change the way I need to do the cascading NS_SUCCEEDED/failure conditions?  What's a good file to model that after?

Comment 10

[Joe Drew (not getting mail)]

I believe that, if you're using pref observers, you just initialize the variable to some sane default (eg 15000), and then handle it being changed at any later time within PrefChanged as in nsHttpHandler. So you won't need to do the cascading NS_SUCCEEDED blocks.

Comment 11

[deadfones]

Attachment: new patch that uses a pref observer instead

I used nsCookiePermission more than nsHttpHandler because it was simpler.  I considered using CLAMP from nsHttpHandler but ultimately didn't.  The effect of different values are: negative number: never discard, 0: discard immediately, positive: normal behavior.  If there's not a pref defined it defaults to 15000.

This is my foray into C so don't give too much credence to the code.  Luckily most of it was copy paste.

Comment 12

[deadfones]

It just occurred to me that a small negative number would actually be close 0xFFFFFFFF ms since mDiscardTimerMS is PRUint32, what the timer natively accepts.  I mention this because I thought there might be some concern about input validation.

Comment 13

[deadfones]

Feedback?  Do I need to do anything else?

Comment 14

[Joe Drew (not getting mail)]

Comment on attachment 363580 [details] [diff] [review]
new patch that uses a pref observer instead

I need to review it. I'll get to it soon!

Comment 15

[deadfones]

(In reply to comment #14)
> (From update of attachment 363580 [details] [diff] [review])
> I need to review it. I'll get to it soon!

Thanks Joe.  No hurry.

Comment 16

[Joe Drew (not getting mail)]

Comment on attachment 363580 [details] [diff] [review]
new patch that uses a pref observer instead

Sadly I've almost certainly caused this patch to bitrot - can you update to latest mozilla-central and upload an updated patch? (Sorry for taking so long!)

>+#define IMAGE_CACHE_DISCARD_TIMER   "image.cache.discard_timer"

Add some meta-information in the name? IMAGE_CACHE_DISCARD_TIMER_PREF_NAME - or shorter if possible :)

>+#define PREF_CHANGED(_P) (!aPref || !strcmp(aPref, _P))

no need for this - just inline the real code, since you're only using it once.

>+  NS_ASSERTION(!nsCRT::strcmp(NS_PREFBRANCH_PREFCHANGE_TOPIC_ID, aTopic),
>+               "unexpected topic - we only deal with pref changes!");

You can just change this to bare strcmp.

> #define NS_IMGCONTAINER_CID \
> { /* 27f0682c-ff64-4dd2-ae7a-668e59f2fd38 */         \
>      0x27f0682c,                                     \
>      0xff64,                                         \
>      0x4dd2,                                         \
>     {0xae, 0x7a, 0x66, 0x8e, 0x59, 0xf2, 0xfd, 0x38} \
> }

Because we're changing the class's implementation (by adding nsIObserver to its parent classes), we'll need to change the CID so binary components that require a specific implementation won't break.

>+  PRUint32                   mDiscardTimerMS;

Can you make this static, and just initialize it to 15000 in the .cpp file? That way we don't make every imgContainer object bigger.

>+// Timer, in ms, to discard decoded images from cache 
>+pref("image.cache.discard_timer", 15000);

We probably don't need this in the default all.js - we'll default to 15s unless someone adds the pref. So remove this. (Test to make sure we don't have issues when adding it in through the UI, though!)

Comment 17

[Joe Drew (not getting mail)]

(In reply to comment #16)
> > #define NS_IMGCONTAINER_CID \
> > { /* 27f0682c-ff64-4dd2-ae7a-668e59f2fd38 */         \
> >      0x27f0682c,                                     \
> >      0xff64,                                         \
> >      0x4dd2,                                         \
> >     {0xae, 0x7a, 0x66, 0x8e, 0x59, 0xf2, 0xfd, 0x38} \
> > }
> 
> Because we're changing the class's implementation (by adding nsIObserver to its
> parent classes), we'll need to change the CID so binary components that require
> a specific implementation won't break.

This actually might be incorrect - Vlad?

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I think that's incorrect -- binary components should be calling QI to get the interface that they want instead of casting directly, so adding something to the inheritance mix shouldn't break them.  If any are doing it wrong, then yeah, they'd break, but they were wrong to begin with :)

Comment 19

[deadfones]

Attachment: new patch

I think I've addressed the problems, and didn't do anything with NS_IMGCONTAINER_CID.

>Add some meta-information in the name? IMAGE_CACHE_DISCARD_TIMER_PREF_NAME - or
shorter if possible :)

Not sure exactly what you want here, but I changed it to IMAGE_CACHE_DISCARD_TIMER_MS_PREF

Comment 20

[Joe Drew (not getting mail)]

Comment on attachment 371013 [details] [diff] [review]
new patch

Looks great!

Comment 21

[Serge Gautherie (:sgautherie)]

'deadfones#gmail.com', may I suggest you to file your (real) name in BugZilla?

Comment 22

[Bobby Holley (:bholley)]

wouldn't it be better for this patch to set a default in all.js instead of hard-coding the default as a static var? If I understand correctly, currently the pref won't show up in about:config unless it's explicitly set, which seems like worse user experience IMO than being able to view its default value.

Joe? Thoughts?

Comment 23

[Joe Drew (not getting mail)]

I don't think so. We definitely have to have a default value in case someone changes all.js, so the static won't go away, and I'm not too convinced that this will need to be modified by many/any people (we should just have the ability to change it for embedders/etc).

Comment 24

[Dão Gottwald [:dao]]

(In reply to comment #23)
> I don't think so. We definitely have to have a default value in case someone
> changes all.js, so the static won't go away

Would "someone" be a mozilla contributor or a user who destroys his local program files? FWIW, we generally don't protect against either case for prefs that have a default value in all.js.

Comment 25

[Joe Drew (not getting mail)]

I'm thinking more like a Fennec-style project who might not have their own pref.

Comment 26

[Dão Gottwald [:dao]]

Fennec has it, though: fennec/xulrunner/greprefs/all.js
all.js is pretty much mandatory, I think.

Comment 27

[Joe Drew (not getting mail)]

This has almost certainly been bitrotted by my changes in bug 753, and will be bitrotted even harder by Bobby Holley in bug 435296.

deadfones, if you update your patch to apply to current hg trunk (or tell me it applies as-is), I'll get it pushed before any further changes happen; if you want to update it to Bobby's work in 435926, we can set a bug dependency and I'll make sure it gets in after Bobby's done.

Sorry for the inaction on this - checkin-needed doesn't always operate magically. :)

Comment 28

[Bobby Holley (:bholley)]

It'd be a bit easier for me if this went on top of my patch rather than underneath it.

deadfones - you're of course free to do whichever, but if you don't feel like updating your patch, I'll take personal responsibility for updating it on top of bug 435296 and landing it (under your name of course).

Comment 29

[deadfones]

Bobby, please just update the patch on your own.

BTW, in addition to embedded, I see this as useful to people with 12gb of ram who browse image heavy sites and are ocd about the re-decode stutter.

Comment 30

[Bobby Holley (:bholley)]

cool - marking this as blocked by decode on draw.

Also, you might be happy to know that the stutter will be completely eliminated with the patch in bug 435296. Decodes are progressive rather than all-at-once, and we won't be discarding visible images.

This won't be there for 3.6, but should be for whatever release follows it.

Comment 31

[Bobby Holley (:bholley)]

@@ -124,16 +130,47 @@ NS_IMETHODIMP imgContainer::Init(PRInt32
 
   mSize.SizeTo(aWidth, aHeight);
   
   // As we are reloading it means we are no longer in 'discarded' state
   mDiscarded = PR_FALSE;
 
   mObserver = do_GetWeakReference(aObserver);
   
+  nsCOMPtr<nsIPrefBranch2> prefBranch = do_GetService(NS_PREFSERVICE_CONTRACTID);
+  if (prefBranch) {
+      prefBranch->AddObserver(IMAGE_CACHE_DISCARD_TIMER_MS_PREF, this, PR_FALSE);
+
+      PrefChanged(prefBranch, nsnull);
+  }
+  
+  return NS_OK;
+}

This re-queries the pref _each_ time a new image is created, no? I'm fixing that in the updated version of the patch (to be posted shortly).

Comment 32

[Bobby Holley (:bholley)]

Attachment: updated patch on top of bugs 435296, 512269, and 511899

updated patch. I'll flag joe and deadfones for review once the dependent bugs land.

Comment 33

[Bobby Holley (:bholley)]

Attachment: patch v2 (fixes a small indentation problem)

fixed a small indentation problem

Comment 34

[Bobby Holley (:bholley)]

Attachment: tests v1

Added tests. Here I also took the liberty of fixing the 20 second test (which previously waited for the discard timeout) to dynamically set the discard timer to something more reasonable. And of course, we also test that the pref itself works.

I'm still pretty strongly in favor of sticking this in all.js (though the meaning might change a bit once we get rid of individual discard timers and do everything through the cache in bug 502694). I don't think this is any less useful or intuitive than image.cache.timeweight, and deadfones has certainly proven that some users care about it (though the impact of discards will, of course, be mitigated by bug 435296).

Joe - thoughts?

Comment 35

[Bobby Holley (:bholley)]

Attachment: tests v2

updated tests

Comment 36

[Joe Drew (not getting mail)]

Putting it in all.js is just fine.

Comment 38

[Bobby Holley (:bholley)]

This should probably go on top of the work I'm doing in bug 573629 and be folded into imgRequest.

Comment 39

[Bobby Holley (:bholley)]

Attachment: patch v3

At long last, an updated version of this patch. This needs to be applied on top of the patch in bug 502694, so it can't land until that lands. Hopefully should make it into ff4 beta 2.

I'll flag for review once I get an r+ for bug 502694 (hopefully monday). Sorry for all the delay deadfones :(

Comment 40

[Jeff Muizelaar [:jrmuizel]]

It would be nice to get a summary of what's changing here.

In the new version it looks like the timer ticks even when there is no work to do, but in the old version it didn't?

Comment 41

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #40)
> It would be nice to get a summary of what's changing here.
> 
> In the new version it looks like the timer ticks even when there is no work to
> do, but in the old version it didn't?

This comment was meant for a different bug, please it ignore it.

Comment 42

[Bobby Holley (:bholley)]

Comment on attachment 454139 [details] [diff] [review]
patch v3

bug 502694 landed. Flagging joe for review

Comment 43

[Joe Drew (not getting mail)]

Comment on attachment 454139 [details] [diff] [review]
patch v3

>diff --git a/modules/libpr0n/src/imgDiscardTracker.cpp b/modules/libpr0n/src/imgDiscardTracker.cpp
>--- a/modules/libpr0n/src/imgDiscardTracker.cpp
>+++ b/modules/libpr0n/src/imgDiscardTracker.cpp
>@@ -9,21 +9,16 @@
>  *
>  * Software distributed under the License is distributed on an "AS IS" basis,
>  * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>  * for the specific language governing rights and limitations under the
>  * License.
>  *
>  * The Original Code is mozilla.org code.
>  *
>- * The Initial Developer of the Original Code is
>- * Netscape Communications Corporation.
>- * Portions created by the Initial Developer are Copyright (C) 2001
>- * the Initial Developer. All Rights Reserved.
>- *

This initial developer removal is wrong. You should probably copy from the license boilerplate and fill in everything, since you created this as a new file in an earlier commit.

http://www.mozilla.org/MPL/boilerplate-1.1/



>  * Contributor(s):
>  *   Bobby Holley <[e-mail]>
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either the GNU General Public License Version 2 or later (the "GPL"), or
>  * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>  * in which case the provisions of the GPL or the LGPL are applicable instead
>  * of those above. If you wish to allow use of your version of this file only
>@@ -31,25 +26,28 @@
>  * use your version of this file under the terms of the MPL, indicate your
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>+#include "nsIServiceManager.h"
>+#include "nsIPrefService.h"
>+#include "nsIPrefBranch2.h"
>+
> #include "nsComponentManagerUtils.h"
> #include "nsITimer.h"
> #include "imgContainer.h"
> #include "imgDiscardTracker.h"
> 
> static PRBool sInitialized = PR_FALSE;
> static PRBool sTimerOn = PR_FALSE;
>-/* TODO - don't hardcode. See bug 478398. */
>-static PRUint32 sMinDiscardTimeoutMs = 10000;
>+static PRUint32 sMinDiscardTimeoutMs = 10000; /* Default if pref unreadable. */
> static nsITimer *sTimer = nsnull;
> static struct imgDiscardTrackerNode sHead, sSentinel, sTail;
> 
> nsresult
> imgDiscardTracker::Reset(imgDiscardTrackerNode *node)
> {
>   nsresult rv;
>   PRBool isSentinel = (node == &sSentinel);
>@@ -120,16 +118,19 @@ imgDiscardTracker::Initialize()
> 
>   // Set up the list. Head<->Sentinel<->Tail
>   sHead.curr = sTail.curr = sSentinel.curr = nsnull;
>   sHead.prev = sTail.next = nsnull;
>   sHead.next = sTail.prev = &sSentinel;
>   sSentinel.prev = &sHead;
>   sSentinel.next = &sTail;
> 
>+  // Load the timeout
>+  ReloadTimeout();
>+
>   // Create and start the timer
>   nsCOMPtr<nsITimer> t = do_CreateInstance("@mozilla.org/timer;1");
>   NS_ENSURE_TRUE(t, NS_ERROR_OUT_OF_MEMORY);
>   t.forget(&sTimer);
>   rv = TimerOn();
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // Mark us as initialized
>@@ -147,16 +148,47 @@ imgDiscardTracker::Shutdown()
>   if (sTimer) {
>     sTimer->Cancel();
>     NS_RELEASE(sTimer);
>     sTimer = nsnull;
>   }
> }
> 
> /**
>+ * Sets the minimum timeout.
>+ */
>+void
>+imgDiscardTracker::ReloadTimeout()
>+{
>+  nsresult rv;
>+
>+  // read the timeout pref
>+  PRInt32 discardTimeout;
>+  nsCOMPtr<nsIPrefBranch2> branch = do_GetService(NS_PREFSERVICE_CONTRACTID);
>+  rv = branch->GetIntPref(DISCARD_TIMEOUT_PREF, &discardTimeout);
>+
>+  // If we got something bogus, return
>+  if (!NS_SUCCEEDED(rv) || discardTimeout <= 0)
>+    return;
>+
>+  // If the value didn't change, return
>+  if ((PRUint32) discardTimeout == sMinDiscardTimeoutMs)
>+    return;
>+
>+  // Update the value
>+  sMinDiscardTimeoutMs = (PRUint32) discardTimeout;
>+
>+  // If the timer's on, restart the clock to make changes take effect
>+  if (sTimerOn) {
>+    TimerOff();
>+    TimerOn();
>+  }
>+}
>+
>+/**
>  * Enables the timer. No-op if the timer is already running.
>  */
> nsresult
> imgDiscardTracker::TimerOn()
> {
>   // Nothing to do if the timer's already on.
>   if (sTimerOn)
>     return NS_OK;
>diff --git a/modules/libpr0n/src/imgDiscardTracker.h b/modules/libpr0n/src/imgDiscardTracker.h
>--- a/modules/libpr0n/src/imgDiscardTracker.h
>+++ b/modules/libpr0n/src/imgDiscardTracker.h
>@@ -10,21 +10,16 @@
>  *
>  * Software distributed under the License is distributed on an "AS IS" basis,
>  * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>  * for the specific language governing rights and limitations under the
>  * License.
>  *
>  * The Original Code is mozilla.org code.
>  *
>- * The Initial Developer of the Original Code is
>- * Netscape Communications Corporation.
>- * Portions created by the Initial Developer are Copyright (C) 2001
>- * the Initial Developer. All Rights Reserved.
>- *
>  * Contributor(s):
>  *   Bobby Holley <[e-mail]>
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either the GNU General Public License Version 2 or later (the "GPL"), or
>  * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>  * in which case the provisions of the GPL or the LGPL are applicable instead
>  * of those above. If you wish to allow use of your version of this file only
>@@ -35,16 +30,18 @@
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #ifndef __imgDiscardTracker_h__
> #define __imgDiscardTracker_h__
> 
>+#define DISCARD_TIMEOUT_PREF "image.mem.min_discard_timeout_ms"
>+
> class imgContainer;
> class nsITimer;
> 
> struct imgDiscardTrackerNode
> {
>   imgContainer *curr;
>   imgDiscardTrackerNode *prev, *next;
> };
>@@ -60,16 +57,17 @@ struct imgDiscardTrackerNode
>  */
> 
> class imgDiscardTracker
> {
>   public:
>     static nsresult Reset(struct imgDiscardTrackerNode *node);
>     static void Remove(struct imgDiscardTrackerNode *node);
>     static void Shutdown();
>+    static void ReloadTimeout();
>   private:
>     static nsresult Initialize();
>     static nsresult TimerOn();
>     static nsresult TimerOff();
>     static void TimerCallback(nsITimer *aTimer, void *aClosure);
> };
> 
> #endif /* __imgDiscardTracker_h__ */
>diff --git a/modules/libpr0n/src/imgRequest.cpp b/modules/libpr0n/src/imgRequest.cpp
>--- a/modules/libpr0n/src/imgRequest.cpp
>+++ b/modules/libpr0n/src/imgRequest.cpp
>@@ -69,16 +69,18 @@
> #include "nsXPIDLString.h"
> #include "plstr.h" // PL_strcasestr(...)
> #include "nsNetUtil.h"
> #include "nsIProtocolHandler.h"
> 
> #include "nsIPrefService.h"
> #include "nsIPrefBranch2.h"
> 
>+#include "imgDiscardTracker.h"
>+
> #define DISCARD_PREF "image.mem.discardable"
> #define DECODEONDRAW_PREF "image.mem.decodeondraw"
> 
> /* Kept up to date by a pref observer. */
> static PRBool gDecodeOnDraw = PR_FALSE;
> static PRBool gDiscardable = PR_TRUE;
> 
> /*
>@@ -98,16 +100,19 @@ ReloadPrefs(nsIPrefBranch *aBranch)
>   if (NS_SUCCEEDED(rv))
>     gDiscardable = discardable;
> 
>   // Decode-on-draw
>   PRBool decodeondraw;
>   rv = aBranch->GetBoolPref(DECODEONDRAW_PREF, &decodeondraw);
>   if (NS_SUCCEEDED(rv))
>     gDecodeOnDraw = decodeondraw;
>+
>+  // Discard timeout
>+  imgDiscardTracker::ReloadTimeout();
> }
> 
> // Observer
> class imgRequestPrefObserver : public nsIObserver {
> public:
>     NS_DECL_ISUPPORTS
>     NS_DECL_NSIOBSERVER
> };
>@@ -119,17 +124,18 @@ imgRequestPrefObserver::Observe(nsISuppo
>                                 const char      *aTopic,
>                                 const PRUnichar *aData)
> {
>   // Right topic
>   NS_ABORT_IF_FALSE(!strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID), "invalid topic");
> 
>   // Right pref
>   if (strcmp(NS_LossyConvertUTF16toASCII(aData).get(), DISCARD_PREF) &&
>-      strcmp(NS_LossyConvertUTF16toASCII(aData).get(), DECODEONDRAW_PREF))
>+      strcmp(NS_LossyConvertUTF16toASCII(aData).get(), DECODEONDRAW_PREF) &&
>+      strcmp(NS_LossyConvertUTF16toASCII(aData).get(), DISCARD_TIMEOUT_PREF))
>     return NS_OK;
> 
>   // Get the pref branch
>   nsCOMPtr<nsIPrefBranch> branch = do_QueryInterface(aSubject);
>   if (!branch) {
>     NS_WARNING("Couldn't get pref branch within imgRequestPrefObserver::Observe!");
>     return NS_OK;
>   }
>@@ -219,16 +225,17 @@ nsresult imgRequest::Init(nsIURI *aURI,
>   // Register our pref observer if it hasn't been done yet.
>   if (NS_UNLIKELY(!gRegisteredPrefObserver)) {
>     imgRequestPrefObserver *observer = new imgRequestPrefObserver();
>     if (observer) {
>       nsCOMPtr<nsIPrefBranch2> branch = do_GetService(NS_PREFSERVICE_CONTRACTID);
>       if (branch) {
>         branch->AddObserver(DISCARD_PREF, observer, PR_FALSE);
>         branch->AddObserver(DECODEONDRAW_PREF, observer, PR_FALSE);
>+        branch->AddObserver(DISCARD_TIMEOUT_PREF, observer, PR_FALSE);
>         ReloadPrefs(branch);
>         gRegisteredPrefObserver = PR_TRUE;
>       }
>     }
>     else
>       delete observer;
>   }
> 
>diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js
>--- a/modules/libpref/src/init/all.js
>+++ b/modules/libpref/src/init/all.js
>@@ -3114,16 +3114,21 @@ pref("image.http.accept", "image/png,ima
> // Discards inactive image frames and re-decodes them on demand from
> // compressed data.
> pref("image.mem.discardable", false);
> 
> // Prevents images from automatically being decoded on load, instead allowing
> // them to be decoded on demand when they are drawn.
> pref("image.mem.decodeondraw", false);
> 
>+// Minimum timeout for image discarding (in milliseconds). The actual time in
>+// which an image must inactive for it to be discarded will vary between this
>+// value and twice this value.
>+pref("image.mem.min_discard_timeout_ms", 10000);
>+
> // WebGL prefs
> pref("webgl.enabled_for_all_sites", false);
> pref("webgl.software_render", false);
> pref("webgl.osmesalib", "");
> 
> #ifdef XP_WIN
> #ifndef WINCE
> // The default TCP send window on Windows is too small, and autotuning only occurs on receive

Comment 44

[Bobby Holley (:bholley)]

Attachment: patch v4

Updated patch to address joe's comments.

Comment 45

[Bobby Holley (:bholley)]

Pushed this under deadfones' name to mozilla-central:

http://hg.mozilla.org/mozilla-central/rev/97b8d1dd4c65

Leaving this bug open because I still need to unbitrot and land the tests.

Comment 46

[Alfred Kayser]

Some small nits:
1. Instead of !NS_SUCCEEDED(rv) use NS_FAILED(rv)
2. move declaration of rv to its first (and only!) use.

Comment 47

[Bobby Holley (:bholley)]

Attachment: tests v3

Added updated tests, as well as some fixes for other tests now that the pref machinery has landed. Flagging joe for review.

Comment 48

[Joe Drew (not getting mail)]

Comment on attachment 456930 [details] [diff] [review]
tests v3


>diff --git a/modules/libpr0n/test/mochitest/imgutils.js b/modules/libpr0n/test/mochitest/imgutils.js

> // Functions to facilitate getting/setting the discard timer pref
> //
> // Don't forget to reset the pref to the original value!
> //
> // Null indicates no pref set

Update the comments here to be more correct.


>diff --git a/modules/libpr0n/test/mochitest/test_bug478398.html b/modules/libpr0n/test/mochitest/test_bug478398.html

>+  // Wait 3 seconds, then finish the test
>+  setTimeout("finishTest();", 3000);

Why are you waiting 3 seconds? Shouldn't 1 be enough?

Comment 49

[Bobby Holley (:bholley)]

Attachment: tests v4

Updated patch, addressing joe's concerns

Comment 50

[Bobby Holley (:bholley)]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/88283dc6c2f1

Resolving fixed.

Comment 51

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

Seems like this has caused crashes like 
http://crash-stats.mozilla.com/report/index/bp-d8f259a6-c4b2-4b96-a6aa-a123f2100821