Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
869593002:
Skip direct map from apk check for pre-KitKat Samsung. (Closed)
|
Created:
5 years, 11 months ago by simonb (inactive) Modified:
5 years, 10 months ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSkip direct map from apk check for pre-KitKat Samsung.
If the android build version pre-dates KitKat and the device
manufacturer is Samsung, skip the check for mmap exec support.
This avoids triggering a warning on these devices.
The version check is included because these devices do not show
the warning on later OS builds.
BUG=448084
TBR=rmcilroy@chromium.org
Committed: https://crrev.com/78efcc0f7e59612ec717d26ffce92fa643347210
Cr-Commit-Position: refs/heads/master@{#314156}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Update for review feedback #
Total comments: 2
Patch Set 3 : Skip for all pre-KitKat Samsung #Patch Set 4 : Skip check for samsung, register UNKNOWN in UMA #
Total comments: 2
Patch Set 5 : Fix capitalization in a comment. #
Messages
Total messages: 21 (5 generated)
andrewhayden@chromium.org changed reviewers: + andrewhayden@chromium.org
LGTM % minorest of nits https://codereview.chromium.org/869593002/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/869593002/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/Linker.java:202: private static final Set<String> MAP_CHECK_BLACKLIST = In general we are to avoid static initialization because of the overhead during class loading. In this case I think you might legitimately argue that it doesn't matter as the construction MUST take place before anything useful can happen in Chrome, since it's.... the linker. But in theory, this should be lazily initialized. The easiest thing to do here is probably move the entire check into a function that creates this map if it's currently null. Since the map will never change you should be able to get away with a plain old "static Set<String>" without any kind of locking semantics, worst case two threads create identical set and we don't care about that at all. You could also probably just add a one-line comment noting that the linker has to run before anything useful is done so static initialization isn't a major concern, https://codereview.chromium.org/869593002/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/Linker.java:211: private static final int MAP_CHECK_BLACKLIST_EXPIRY = I'd prefer to see the word "min" or "max" used here so it's clear just from reading how the limit works, like: MAX_VERSION_BLACKLIST_REQUIRED ... or ... MIN_VERSION_BLACKLIST_NOT_REQUIRED Minor, your discretion...
https://codereview.chromium.org/869593002/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/869593002/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/Linker.java:202: private static final Set<String> MAP_CHECK_BLACKLIST = On 2015/01/22 17:36:40, Andrew Hayden wrote: > In general we are to avoid static initialization because of the overhead during > class loading. In this case I think you might legitimately argue that it doesn't > matter as the construction MUST take place before anything useful can happen in > Chrome, since it's.... the linker. But in theory, this should be lazily > initialized. > > The easiest thing to do here is probably move the entire check into a function > that creates this map if it's currently null. Since the map will never change > you should be able to get away with a plain old "static Set<String>" without any > kind of locking semantics, worst case two threads create identical set and we > don't care about that at all. > > You could also probably just add a one-line comment noting that the linker has > to run before anything useful is done so static initialization isn't a major > concern, Thanks, done. Locking included to cover the (theoretical) case where one thread reads data from a set whose state is half-populated because another thread is in the process of initializing it. https://codereview.chromium.org/869593002/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/Linker.java:211: private static final int MAP_CHECK_BLACKLIST_EXPIRY = On 2015/01/22 17:36:40, Andrew Hayden wrote: > I'd prefer to see the word "min" or "max" used here so it's clear just from > reading how the limit works, like: > > MAX_VERSION_BLACKLIST_REQUIRED > ... or ... > MIN_VERSION_BLACKLIST_NOT_REQUIRED > > Minor, your discretion... Left as is for now.
https://codereview.chromium.org/869593002/diff/20001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/869593002/diff/20001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/Linker.java:214: android.os.Build.VERSION_CODES.KITKAT; I don't think you can use this on ICS, right? How do we deal with that?
On 2015/01/23 14:59:51, Andrew Hayden wrote: > https://codereview.chromium.org/869593002/diff/20001/base/android/java/src/or... > File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): > > https://codereview.chromium.org/869593002/diff/20001/base/android/java/src/or... > base/android/java/src/org/chromium/base/library_loader/Linker.java:214: > android.os.Build.VERSION_CODES.KITKAT; > I don't think you can use this on ICS, right? How do we deal with that? I think you'll have to hard-code the value in. It should be safe to take the actual integer from http://developer.android.com/reference/android/os/Build.VERSION_CODES.html
https://codereview.chromium.org/869593002/diff/20001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/869593002/diff/20001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/Linker.java:214: android.os.Build.VERSION_CODES.KITKAT; On 2015/01/23 14:59:51, Andrew Hayden wrote: > I don't think you can use this on ICS, right? How do we deal with that? Compile-time constants should be okay. From: http://docs.oracle.com/javase/tutorial/java/javaOO/classvars.html "If a primitive type or a string is defined as a constant and the value is known at compile time, the compiler replaces the constant name everywhere in the code with its value. This is called a compile-time constant."
I like it when I'm wrong about things being hard. Yay! LGTM.
LGTM, but we should try to confirm at least one physical device from a country with a non-latin-based locale still uses "samsung" in the manufacturer string. I'm not sure if we can rely on this being the english word "samsung" in all countries...
simonb@chromium.org changed reviewers: + petrcermak@chromium.org, rmcilroy@chromium.org
LGTM (with one nit) https://codereview.chromium.org/869593002/diff/60001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/869593002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:178: // If the android build version pre-dates KitKat and the device nit: s/android/Android/g
https://codereview.chromium.org/869593002/diff/60001/base/android/java/src/or... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/869593002/diff/60001/base/android/java/src/or... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:178: // If the android build version pre-dates KitKat and the device On 2015/01/31 03:29:14, petrcermak wrote: > nit: s/android/Android/g Done.
Confirmed as fixing the issue for available affected devices, committing: https://code.google.com/p/chromium/issues/detail?id=448084#c70
The CQ bit was checked by simonb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/869593002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by simonb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/869593002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/78efcc0f7e59612ec717d26ffce92fa643347210 Cr-Commit-Position: refs/heads/master@{#314156} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
