Closed Bug 661351 Opened 8 years ago Closed 8 years ago

Handle errors in sysconf in PR_GetPhysicalMemorySize on Android 2.2

Categories

(NSPR :: NSPR, defect, P2)

All
Android
defect

Tracking

(firefox5 wontfix, firefox6 wontfix, firefox7 fixed)

RESOLVED FIXED
Tracking Status
firefox5 --- wontfix
firefox6 --- wontfix
firefox7 --- fixed

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

(Keywords: regression)

Attachments

(2 files, 3 obsolete files)

Attached patch NSPR patch (obsolete) — Splinter Review
+++ 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
Attachment #536719 - Flags: review?(wtc)
Attached patch Places patch (obsolete) — Splinter Review
This patch cleans up some Places code from bug 660036 that works around bad values from PR_GetPhysicalMemorySize.
Assignee: wtc → mbrubeck
Status: NEW → ASSIGNED
Attachment #536722 - Flags: review?(mak77)
Comment on attachment 536719 [details] [diff] [review]
NSPR patch

r=wtc.  You can also test >= 0 instead.
Attachment #536719 - Flags: review?(wtc) → review+
Attached patch NSPR patch (obsolete) — Splinter Review
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.)
Attachment #536719 - Attachment is obsolete: true
Attachment #536782 - Flags: review+
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.
Attachment #536782 - Attachment is obsolete: true
Priority: -- → P2
Target Milestone: --- → 4.8.9
Version: 4.8.9 → other
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?
Attachment #536722 - Flags: review?(mak77) → review+
(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.)
Attachment #536722 - Attachment is obsolete: true
Attachment #536873 - Flags: review+
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.
Keywords: checkin-needed
Places patch landed on mozilla-central for Firefox 7:
http://hg.mozilla.org/mozilla-central/rev/fd50260987f4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
what's the c-n for?
All patches are checked in. Removing checkin-needed.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.