Last Comment Bug 661351 - Handle errors in sysconf in PR_GetPhysicalMemorySize on Android 2.2
: Handle errors in sysconf in PR_GetPhysicalMemorySize on Android 2.2
Status: RESOLVED FIXED
: regression
Product: NSPR
Classification: Components
Component: NSPR (show other bugs)
: other
: All Android
: P2 normal (vote)
: 4.8.9
Assigned To: Matt Brubeck (:mbrubeck)
:
Mentors:
Depends on: 660036
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-01 13:30 PDT by Matt Brubeck (:mbrubeck)
Modified: 2013-12-27 14:19 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
wontfix
fixed


Attachments
NSPR patch (820 bytes, patch)
2011-06-01 13:30 PDT, Matt Brubeck (:mbrubeck)
wtc: review+
Details | Diff | Splinter Review
Places patch (4.51 KB, patch)
2011-06-01 13:38 PDT, Matt Brubeck (:mbrubeck)
mak77: review+
Details | Diff | Splinter Review
NSPR patch (837 bytes, patch)
2011-06-01 16:30 PDT, Matt Brubeck (:mbrubeck)
mbrubeck: review+
Details | Diff | Splinter Review
NSPR patch by Matt Brubeck (checked in) (931 bytes, patch)
2011-06-01 19:08 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Places patch for checkin (4.52 KB, patch)
2011-06-02 06:53 PDT, Matt Brubeck (:mbrubeck)
mbrubeck: review+
Details | Diff | Splinter Review

Description Matt Brubeck (:mbrubeck) 2011-06-01 13:30:53 PDT
Created attachment 536719 [details] [diff] [review]
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) 2011-06-01 13:38:56 PDT
Created attachment 536722 [details] [diff] [review]
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 2011-06-01 15:00:02 PDT
Comment on attachment 536719 [details] [diff] [review]
NSPR patch

r=wtc.  You can also test >= 0 instead.
Comment 3 Matt Brubeck (:mbrubeck) 2011-06-01 16:30:25 PDT
Created attachment 536782 [details] [diff] [review]
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 2011-06-01 19:08:17 PDT
Created attachment 536812 [details] [diff] [review]
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) 2011-06-01 20:09:21 PDT
Pushed to Try: http://tbpl.mozilla.org/?tree=Try&rev=b864ab1c1741
Comment 6 Marco Bonardo [::mak] 2011-06-02 01:44:57 PDT
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) 2011-06-02 06:53:28 PDT
Created attachment 536873 [details] [diff] [review]
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 Ted Mielczarek [:ted.mielczarek] 2011-06-02 16:35:51 PDT
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] 2011-06-02 16:56:39 PDT
NSPR part landed
http://hg.mozilla.org/mozilla-central/rev/6bb5f12c636f
Comment 10 Matt Brubeck (:mbrubeck) 2011-06-06 09:29:37 PDT
Places patch landed on mozilla-central for Firefox 7:
http://hg.mozilla.org/mozilla-central/rev/fd50260987f4
Comment 11 Marco Bonardo [::mak] 2011-08-18 01:51:50 PDT
what's the c-n for?
Comment 12 Matt Brubeck (:mbrubeck) 2011-08-18 08:30:24 PDT
All patches are checked in. Removing checkin-needed.

Note You need to log in before you can comment on or make changes to this bug.