Handle errors in sysconf in PR_GetPhysicalMemorySize on Android 2.2

RESOLVED FIXED in Firefox 7

Status

NSPR
NSPR
P2
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: mbrubeck, Assigned: mbrubeck)

Tracking

({regression})

other
4.8.9
All
Android
regression

Firefox Tracking Flags

(firefox5 wontfix, firefox6 wontfix, firefox7 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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
Attachment #536719 - Flags: review?(wtc)
(Assignee)

Comment 1

6 years ago
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.
Assignee: wtc → mbrubeck
Status: NEW → ASSIGNED
Attachment #536722 - Flags: review?(mak77)

Comment 2

6 years ago
Comment on attachment 536719 [details] [diff] [review]
NSPR patch

r=wtc.  You can also test >= 0 instead.
Attachment #536719 - Flags: review?(wtc) → review+
(Assignee)

Comment 3

6 years ago
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.)
Attachment #536719 - Attachment is obsolete: true
Attachment #536782 - Flags: review+

Comment 4

6 years ago
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.
Attachment #536782 - Attachment is obsolete: true

Updated

6 years ago
Priority: -- → P2
Target Milestone: --- → 4.8.9
Version: 4.8.9 → other
(Assignee)

Comment 5

6 years ago
Pushed to Try: http://tbpl.mozilla.org/?tree=Try&rev=b864ab1c1741
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+
(Assignee)

Comment 7

6 years ago
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.)
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.
NSPR part landed
http://hg.mozilla.org/mozilla-central/rev/6bb5f12c636f
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 10

6 years ago
Places patch landed on mozilla-central for Firefox 7:
http://hg.mozilla.org/mozilla-central/rev/fd50260987f4
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
status-firefox5: affected → wontfix
status-firefox6: affected → wontfix
status-firefox7: affected → fixed
Resolution: --- → FIXED
what's the c-n for?
(Assignee)

Comment 12

6 years ago
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.