661351 - Handle errors in sysconf in PR_GetPhysicalMemorySize on Android 2.2

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P2)

Description

[Matt Brubeck (:mbrubeck)]

Attachment: NSPR patch

+++ This bug was initially created as a clone of Bug #660036 +++

Android 2.2 and earlier has a bug that causes sysconf(_SC_PHYS_PAGES) to return a negative number.  This causes GetPhysicalMemorySize to return a garbage value, which resulted in bug 660036.

This patch catches the error and returns 0 from GetPhysicalMemorySize.

The underlying bug is fixed in later versions of Android:
http://android.git.kernel.org/?p=platform/bionic.git;a=commitdiff;h=038fbae518e904c7aba64779714a22dbeeb90887

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: Places patch

This patch cleans up some Places code from bug 660036 that works around bad values from PR_GetPhysicalMemorySize.

Comment 2

[Wan-Teh Chang]

Comment on attachment 536719 [details] [diff] [review]
NSPR patch

r=wtc.  You can also test >= 0 instead.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: NSPR patch

Same patch generated against NSPR CVS; carrying r+.  WTC, can you please check this in to upstream NSPR?  (I'm not 100% sure of the process here - please let me know if I should do anything else.)

Comment 4

[Wan-Teh Chang]

Attachment: NSPR patch by Matt Brubeck (checked in)

Patch checked in on the NSPR trunk (NSPR 4.8.9).

Checking in prsystem.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prsystem.c,v  <--  prsystem.c
new revision: 3.36; previous revision: 3.35
done

Ted (:luser) will push a new NSPR CVS tag to m-c (see bug 626035
comment 90), which will include this patch.

Comment 5

[Matt Brubeck (:mbrubeck)]

Pushed to Try: http://tbpl.mozilla.org/?tree=Try&rev=b864ab1c1741

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 536722 [details] [diff] [review]
Places patch

Review of attachment 536722 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed

::: toolkit/components/places/nsNavHistory.cpp
@@ +135,5 @@
>  // Out of this cache, SQLite will use at most the size of the database file.
>  #define DATABASE_DEFAULT_CACHE_TO_MEMORY_PERCENTAGE 6
>  
> +// If the physical memory size is not available, use MEMSIZE_FALLBACK_BYTES
> +// instead.  [Must stay in sync with the code in nsPlacesExpiration.js]

please remove the square brackets as you did in the original patch.

::: toolkit/components/places/nsPlacesExpiration.js
@@ +717,5 @@
>      catch(e) {}
>  
>      if (this._urisLimit < 0) {
> +      // If physical memory size is not available, use MEMSIZE_FALLBACK_BYTES
> +      // instead.  [Must stay in sync with the code in nsNavHistory.cpp]

ditto

@@ +723,5 @@
>  
>        // The preference did not exist or has a negative value, so we calculate a
>        // limit based on hardware.
>        let memsize = this._sys.getProperty("memsize"); // Memory size in bytes.
> +      if (memsize <= 0)

why <= 0 here?

Comment 7

[Matt Brubeck (:mbrubeck)]

Attachment: Places patch for checkin

(In reply to comment #6)
> please remove the square brackets as you did in the original patch.

Done.

> > +      if (memsize <= 0)
> 
> why <= 0 here?

We need the value to be positive.  In the C++ code, the variable is PRUint64 which is guaranteed to be non-negative.  In the JavaScript the language doesn't give me this guarantee so I am just being paranoid to guard against any similar future bug.  (If we had ASSERT in JavaScript, this could be an debug-only assertion instead, since it should never happen.)

Comment 8

[(not currently active) Ted Mielczarek]

This patch is in the NSPR_4_8_9_BETA3 tag. glandium is probably going to merge that to mozilla-central to pick up bug 626035.

Comment 9

[Mike Hommey [:glandium]]

NSPR part landed
http://hg.mozilla.org/mozilla-central/rev/6bb5f12c636f

Comment 10

[Matt Brubeck (:mbrubeck)]

Places patch landed on mozilla-central for Firefox 7:
http://hg.mozilla.org/mozilla-central/rev/fd50260987f4

Comment 11

[Marco Bonardo [:mak]]

what's the c-n for?

Comment 12

[Matt Brubeck (:mbrubeck)]

All patches are checked in. Removing checkin-needed.