Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
156353003:
Attempt to fix crash in base::android::MethodID::Get() (Closed)
|
Created:
6 years, 10 months ago by henrika (OOO until Aug 14) Modified:
6 years, 10 months ago Reviewers:
cimamoglu (inactive), tommi (sloooow) - chröme, Ami GONE FROM CHROMIUM, bulach, ajm, whywhat CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAttempt to fix crash in base::android::MethodID::Get()
BUG=336600
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249756
Patch Set 1 : improved shouldUseAcousticEchoCanceler #
Total comments: 2
Patch Set 2 : tommi@ plus try-catch #
Total comments: 3
Patch Set 3 : simplified try-catch #
Total comments: 4
Patch Set 4 : modified usage of mIsInitialized #
Total comments: 2
Patch Set 5 : nit #
Total comments: 9
Patch Set 6 : added more sanity and cleanup #
Total comments: 1
Patch Set 7 : Extra BT check #Patch Set 8 : fixed findbugs issue #
Total comments: 3
Patch Set 9 : @tommi #Patch Set 10 : findbugs fix #
Total comments: 29
Patch Set 11 : Ami@ #Patch Set 12 : Ami@ #Messages
Total messages: 43 (0 generated)
https://codereview.chromium.org/156353003/diff/20001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/20001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:413: if (Build.VERSION.SDK_INT < Build.VERSION_CODES.JELLY_BEAN) { As discussed offline, I think this is not the right solution and imho overkill. Here's what I suppose instead and I think this will correctly ensure that we only call AcousticEchoCanceler.isAvailable() on platforms that we have whitelisted: return Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN && (Build.MODEL.equals("SM-T310R") || // Galaxy Tab 3 7.0 ...) && AcousticEchoCanceler.isAvailable(); Note that the only change here is moving the isAvailable call behind all the other checks but that's the key. The current solution adds an exception handler for whitelisted devices and I don't think that's what we want to do. Either they are whitelisted and things are expected to work, or they should not be whitelisted.
Also added some try-catch as discussed offline. tommi@: PTAL https://codereview.chromium.org/156353003/diff/20001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/20001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:413: if (Build.VERSION.SDK_INT < Build.VERSION_CODES.JELLY_BEAN) { Thanks for the detailed feedback. I was trying to combine earlier feedback from bulach@ with what we discussed offline. Will modify.
https://codereview.chromium.org/156353003/diff/60001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/60001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:193: try { Is there value in having multiple catch handlers vs one? I.e. will we somehow get information about what catch handler we hit from the field?
https://codereview.chromium.org/156353003/diff/60001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/60001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:193: try { Not 100% if I answer your question, but: try { test1(); } catch (Exception e) { logwtf("Exception #1: " + e.getMessage()); return; } try { test2(); } catch (Exception e) { logwtf("Exception #2: " + e.getMessage()); return; } private void test1() { // throw new IllegalArgumentException("*** TEST 1 ****"); } private void test2() { throw new IllegalArgumentException("*** TEST 2 ****"); } will result in: non-crashing tests but logcat printouts like: F/AudioManagerAndroid( 8548): Exception #2: *** TEST 2 ****
Yes, I think I get that :) My question is: Who will see those printouts?
Point taken, will simplify.
tommi@: PTAL
https://codereview.chromium.org/156353003/diff/130001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/130001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:202: mIsInitialized = true; Since init() returns void (there's no way to know it failed), do we also need to have checks for mIsInitialized in methods that that AudioManager is going to be calling? Or to think about it another way, is the class completely not initialized if it failed to initialize the settings observer? If the answer to that is 'no' then, we should set mIsInitialized to true before the 'try' https://codereview.chromium.org/156353003/diff/130001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:238: private void setCommunicationAudioModeOn(boolean on) { is it ok to call this method if mIsInitialized==false? same for other methods.
Did an attempt to improve somewhat. Please note that I am not sure if the added try-catch part in init is even needed. https://codereview.chromium.org/156353003/diff/130001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/130001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:202: mIsInitialized = true; Good question. Note that I am only guessing when adding try-catch here. Not sure at all if it will solve the crash. Ensured that we always set initialized to true when this method is called. Even if the "try-part" fails, we are still initialized. The "only" thing that does not work is that we will not be notified about volume changes. https://codereview.chromium.org/156353003/diff/130001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:238: private void setCommunicationAudioModeOn(boolean on) { Yes it is. It does unrelated parts that are not related to init(). Checked other methods and added check if needed.
https://codereview.chromium.org/156353003/diff/170001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/170001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:217: mSettingsObserverThread.quit(); check for null first?
Added suggested NULL check. bulach@: please take a look as well. https://codereview.chromium.org/156353003/diff/170001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/170001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:217: mSettingsObserverThread.quit(); On 2014/02/06 16:32:28, tommi wrote: > check for null first? Done.
adding a few more folks with experience on SDK detection... https://codereview.chromium.org/156353003/diff/200001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/200001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:437: return (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN && I would suggest doing the if SDK_INT in its own block... I don't quite know how java evaluates the full expression and resolves the symbols, so it wouldn't hurt to call it out explicit..
@bulach: FWIW Java evaluates in the intuitive way, and e.g. in the expression if (something() && Foo.otherthing()) if something() evaluates to false then Foo is never referenced (so for example any static initializer blocks it has do not get executed). To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/156353003/diff/200001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/200001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:437: return (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN && On 2014/02/06 16:48:14, bulach wrote: > I would suggest doing the if SDK_INT in its own block... > I don't quite know how java evaluates the full expression and resolves the > symbols, so it wouldn't hurt to call it out explicit.. Do you know if symbol resolution actually has anything effect? primiano just pointed out that we have had a reference to this type for some time without (afaik) problems: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android... If that really is the problem, then I suspect we might have plenty of issues coming our way :| and it's a very hard thing to protect against. Regarding doing the SDK_INT check explicitly, that to me feels like we don't trust the language and we're just guessing. Here's the language spec: http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.23 I would be very surprised (i.e. the engine is broken!) if moving the condition check actually makes a difference. However, I think you were right in pointing out that we were making the |isAvailable()| call on all devices that met the version check but not the whitelist and I think that was a genuine bug.
https://codereview.chromium.org/156353003/diff/200001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/200001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:446: AcousticEchoCanceler.isAvailable()); I'm not sure what this rearrangement is meant to achieve - some of the crashes report are from e.g. Galaxy Note 3 running KK, so it's not like the JVM won't see this code execute...
On Thu, Feb 6, 2014 at 9:26 AM, <tommi@chromium.org> wrote: > Do you know if symbol resolution actually has anything effect? > If by "symbol resolution" you mean class loading, then yes, it has the effect of executing static initializer blocks in the class, e.g.: class Foo { static { doSomethingExactlyOnceAndBeforeAnythingElseInThisClassExecutes(); } public static void bar(); } when you run Foo.bar() the long method name will be executed first (assuming this is the first time the class was touched). Assigning null to a member of type Foo does not trigger this execution. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/156353003/diff/200001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/200001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:446: AcousticEchoCanceler.isAvailable()); On 2014/02/06 17:31:35, Ami Fischman wrote: > I'm not sure what this rearrangement is meant to achieve - some of the crashes > report are from e.g. Galaxy Note 3 running KK, so it's not like the JVM won't > see this code execute... Well, the rearrangement means that isAvailable won't be called when running on devices that aren't in the list. However, have we actually seen any crashes in this particular function? I'm only aware of crashes that point to the init() function but granted I haven't done much spelunking.
Thanks Ami - re; the "symbol resolution", that was my expectation and that's why I'm wondering if we're barking up the wrong tree. On Thu, Feb 6, 2014 at 6:34 PM, Ami Fischman <fischman@chromium.org> wrote: > > On Thu, Feb 6, 2014 at 9:26 AM, <tommi@chromium.org> wrote: > >> Do you know if symbol resolution actually has anything effect? >> > > If by "symbol resolution" you mean class loading, then yes, it has the > effect of executing static initializer blocks in the class, e.g.: > class Foo { > static { > doSomethingExactlyOnceAndBeforeAnythingElseInThisClassExecutes(); > } > public static void bar(); > } > when you run Foo.bar() the long method name will be executed first > (assuming this is the first time the class was touched). > > Assigning null to a member of type Foo does not trigger this execution. > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Feb 6, 2014 at 9:39 AM, <tommi@chromium.org> wrote: > On 2014/02/06 17:31:35, Ami Fischman wrote: > >> I'm not sure what this rearrangement is meant to achieve - some of the >> > crashes > >> report are from e.g. Galaxy Note 3 running KK, so it's not like the >> > JVM won't > >> see this code execute... >> > > Well, the rearrangement means that isAvailable won't be called when > running on devices that aren't in the list. Right; my point is that the linked bug says we need to deal with problems occurring on devices that _are_ on the list, so doing less work on devices _not_ on the list isn't going to help :) To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
yes, you're right. I figured this part of the CL was doing a "might as well fix this too" type of fix that had been pointed out by Markus in another code review. On Thu, Feb 6, 2014 at 6:43 PM, Ami Fischman <fischman@chromium.org> wrote: > > On Thu, Feb 6, 2014 at 9:39 AM, <tommi@chromium.org> wrote: > >> On 2014/02/06 17:31:35, Ami Fischman wrote: >> >>> I'm not sure what this rearrangement is meant to achieve - some of the >>> >> crashes >> >>> report are from e.g. Galaxy Note 3 running KK, so it's not like the >>> >> JVM won't >> >>> see this code execute... >>> >> >> Well, the rearrangement means that isAvailable won't be called when >> running on devices that aren't in the list. > > > Right; my point is that the linked bug says we need to deal with problems > occurring on devices that _are_ on the list, so doing less work on devices > _not_ on the list isn't going to help :) > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/156353003/diff/200001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/200001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:437: return (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN && On 2014/02/06 17:26:06, tommi wrote: > On 2014/02/06 16:48:14, bulach wrote: > > I would suggest doing the if SDK_INT in its own block... > > I don't quite know how java evaluates the full expression and resolves the > > symbols, so it wouldn't hurt to call it out explicit.. > > Do you know if symbol resolution actually has anything effect? > > primiano just pointed out that we have had a reference to this type for some > time without (afaik) problems: > > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android... > > If that really is the problem, then I suspect we might have plenty of issues > coming our way :| and it's a very hard thing to protect against. > > Regarding doing the SDK_INT check explicitly, that to me feels like we don't > trust the language and we're just guessing. > > Here's the language spec: > http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.23 > > I would be very surprised (i.e. the engine is broken!) if moving the condition > check actually makes a difference. > > However, I think you were right in pointing out that we were making the > |isAvailable()| call on all devices that met the version check but not the > whitelist and I think that was a genuine bug. I'm not a vm developer :) but I think we can agree from the crash stack that the class is not being able to load, on devices that are *not* part of this whitelist, right? having said that, is there any concern in fixing this? and while at it, is there any reason not to be extra safe and split the sdk detection from the actual method referencing?
https://codereview.chromium.org/156353003/diff/200001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/200001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:437: return (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN && You mean, break out SDK part in separate static method and call it from this one and return false if SDK says false?
On Thu, Feb 6, 2014 at 10:28 AM, <bulach@chromium.org> wrote: > having said that, is there any concern in fixing this? > I don't mind if this lands, but I'd like a comment re: the guesswork involved (b/c otherwise it will read strange with no context) > and while at it, is there any reason not to be extra safe and split the > sdk detection from the actual method referencing? > Just the usual "we don't make changes we don't understand" :) I won't block landing speculative changes attempting to poke & prod the bug into submission, but I'll be sad if we never understand the root cause. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I agree with Ami. Regarding the "I think we can agree from the crash stack that the class is not being able to load, on devices that are *not* part of this whitelist, right?" I actually don't think I agree and please don't take that the wrong way :) The reason I'm not so sure about that is because it still looks to me that the problem occurs during the call to init() and init() is called _after_ the object has been constructed (i.e. successfully loaded). Having said that though, I may be making some assumptions that don't hold water such as assuming the constructing the object will throw an exception if it fails. Maybe that's not so and we have a NULL object that we're trying to invoke init() on? On Thu, Feb 6, 2014 at 7:51 PM, Ami Fischman <fischman@chromium.org> wrote: > > On Thu, Feb 6, 2014 at 10:28 AM, <bulach@chromium.org> wrote: > >> having said that, is there any concern in fixing this? >> > > I don't mind if this lands, but I'd like a comment re: the guesswork > involved (b/c otherwise it will read strange with no context) > > >> and while at it, is there any reason not to be extra safe and split the >> sdk detection from the actual method referencing? >> > > Just the usual "we don't make changes we don't understand" :) > > I won't block landing speculative changes attempting to poke & prod the > bug into submission, but I'll be sad if we never understand the root cause. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/02/06 18:51:31, Ami Fischman wrote: > On Thu, Feb 6, 2014 at 10:28 AM, <mailto:bulach@chromium.org> wrote: > > > having said that, is there any concern in fixing this? > > > > I don't mind if this lands, but I'd like a comment re: the guesswork > involved (b/c otherwise it will read strange with no context) > as I said, I'm not a vm expert :) but all examples of SDK detection I seen put the condition test in their own block: http://developer.android.com/training/basics/supporting-devices/platforms.html > > > and while at it, is there any reason not to be extra safe and split the > > sdk detection from the actual method referencing? > > > > Just the usual "we don't make changes we don't understand" :) > > I won't block landing speculative changes attempting to poke & prod the bug > into submission, but I'll be sad if we never understand the root cause. I think we do have a good understanding of the root cause: - on a few devices, it fails to load this class. I agree we don't have enough information to exactly pinpoint where, so yes, there's a speculative aspect to it... however, seeing it as one of the top crashes, I'd rather err on the safer side (I hope you'll agree none of the suggestions are _incorrect_) than not doing any changes.. > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Marcus can you elaborate on how we know that the class is failing to load? See my earlier comment on the same. On Feb 6, 2014 8:06 PM, <bulach@chromium.org> wrote: > On 2014/02/06 18:51:31, Ami Fischman wrote: > >> On Thu, Feb 6, 2014 at 10:28 AM, <mailto:bulach@chromium.org> wrote: >> > > > having said that, is there any concern in fixing this? >> > >> > > I don't mind if this lands, but I'd like a comment re: the guesswork >> involved (b/c otherwise it will read strange with no context) >> > > > as I said, I'm not a vm expert :) > but all examples of SDK detection I seen put the condition test in their > own > block: > http://developer.android.com/training/basics/supporting- > devices/platforms.html > > > > > > and while at it, is there any reason not to be extra safe and split the >> > sdk detection from the actual method referencing? >> > >> > > Just the usual "we don't make changes we don't understand" :) >> > > I won't block landing speculative changes attempting to poke & prod the >> bug >> into submission, but I'll be sad if we never understand the root cause. >> > > I think we do have a good understanding of the root cause: > - on a few devices, it fails to load this class. > > I agree we don't have enough information to exactly pinpoint where, so yes, > there's a speculative aspect to it... > however, seeing it as one of the top crashes, I'd rather err on the safer > side > (I hope you'll agree none of the suggestions are _incorrect_) than not > doing any > changes.. > > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > https://codereview.chromium.org/156353003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Here's my 2 cents: The problem occurs mostly (probably only?) in custom mods. If you look into cynagenmode's code, (not the latest one but one of the recent ones), you can see that they use an old/incomplete version of AcousticEchoCanceler https://github.com/CyanogenMod/android_frameworks_base/blob/cm-9.1.0/media/ja... This revision does not have isAvailable, for instance. Here's the version history of the file from AOSP (cynagenmod's above version uses the oldest revision of the file): https://android.googlesource.com/platform/frameworks/base/+log/cd92588/media/... So if we detect whether the class is complete (through reflection, for instance), we might be able to avoid getting into any problematic state. I discussed the details with Henrik, Tomas and Marcus in chat. I don't know if this would solve the problems, but it could be of help.
lgtm (meetings through the rest of the day), following our long chat... summary: let's be over-defensive, and split / protect any SDK detection. https://codereview.chromium.org/156353003/diff/200001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/200001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:542: BluetoothAdapter btAdapter = null; I'd also suggest encapsulating this in a try..except block and returning false should anything happen...
tommi@: PTAL. Added more sanity and extra safety for BT as well. https://codereview.chromium.org/156353003/diff/200001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/200001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:446: AcousticEchoCanceler.isAvailable()); Based on our off-line discussions yesterday I will make some sort of change that everyone seem OK with. No guarantees. https://codereview.chromium.org/156353003/diff/200001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:542: BluetoothAdapter btAdapter = null; On 2014/02/06 20:28:25, bulach wrote: > I'd also suggest encapsulating this in a try..except block and returning false > should anything happen... Done.
https://codereview.chromium.org/156353003/diff/380001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/380001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:41: private static final boolean DEBUG = true; Will be set to false.
lgtm https://codereview.chromium.org/156353003/diff/480001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/480001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:451: return ((Build.MODEL.equals("SM-T310R") || // Galaxy Tab 3 7.0 nit: remove extra parenthesis https://codereview.chromium.org/156353003/diff/480001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:560: BluetoothManager btManager = fix indent
Done. https://codereview.chromium.org/156353003/diff/480001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/480001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:451: return ((Build.MODEL.equals("SM-T310R") || // Galaxy Tab 3 7.0 On 2014/02/07 13:52:30, tommi wrote: > nit: remove extra parenthesis Done.
OK, guess I'm done then but I must admit that I have no idea if this CL will solve the actual problem. And if it does, I will not understand why it helped. Ami, my plan was to commit today (STO time). Please ping me if you have comments and I will revert and fix.
The CQ bit was checked by henrika@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/henrika@chromium.org/156353003/580001
There's a bug noted below; please fix asap. https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:207: } catch (Exception e) { In general while coding Java (say, when not trying to poke in the dark at an unreproducible bug) "catch (Exception" is a sure sign of something being wrong. E.g. this will black-hole OOM, JVM bugs, etc. This deserves a comment + TODO/crbug to remove once we have data about whether it helps. (ditto for elsewhere in the file where you make changes that would not be acceptable in the normal course of things). https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:208: // It is fine to rely on code below here. s/\./ to detect failure by observing mSettingsObserver==null./ https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:209: logwtf("SettingsObserver exception: " + e.getMessage()); What a terrible helper :) Log has a lesser-known 3-argument variant that accepts an a Throwable as the third parameter and emits useful information to the log (mostly thinking of nested exceptions, which are hidden by getMessage). logwtf("hello" + e.getMessage()); is even more typing than Log.wtf(TAG, "hello", e); ! Not necessarily for this CL, but would be good to clean up. Esp. because while hunting for the crasher, knowing nested exceptions would be super-useful. https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:212: mIsInitialized = true; did you mean to early-return from the catch clause? Because as things stand now you're setting mIsInitialized unconditionally. https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:230: } catch (Exception e) { Why this, considering the crasher being hunted is in init()? https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:238: } catch (Exception e) { ditto https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:440: extra \n https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:456: Build.MODEL.equals("SM-N9005") || // Galaxy Note 3 this was previously sorted by model number, which seems better to me. https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:459: Build.MODEL.equals("Nexus 7") && This is a bug. You changed isAvailable && (modelA || modelB || ...) into modelA || model B || ... && isAvailable but the way operator precedence & short-circuiting works in C-derived languages is that once one of the models matches, the entire expression is considered true and isAvailable is not evaluated. E.g. System.out.println(false || true || false && false); prints "true". Since you're chasing a suspected bug in even mentioning AEC, I suggest you transform this to: if (!Build.MODEL.equals("SM-T310R") && !Build.MODEL.equals("GT-I9300") ... !Build.MODEL.equals("Nexus 7")) { return false; } return AcousticEchoCanceler.isAvailable(); https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:566: btAdapter = null; log? https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:574: btAdapter = null; log? https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:579: return false; Then why not instead simply return false from the catch blocks above? https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:582: android.bluetooth.BluetoothProfile.STATE_DISCONNECTED; this assignment is useless? https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:591: return (btAdapter.isEnabled() && profileConnectionState == isEnabled() is new in this change. Are you worried that an adapter may be CONNECTED but disabled? (worth a comment)
https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:459: Build.MODEL.equals("Nexus 7") && On 2014/02/07 17:06:45, Ami Fischman wrote: > This is a bug. > > You changed > isAvailable && (modelA || modelB || ...) > into > modelA || model B || ... && isAvailable > but the way operator precedence & short-circuiting works in C-derived languages > is that once one of the models matches, the entire expression is considered true > and isAvailable is not evaluated. > > E.g. System.out.println(false || true || false && false); > prints "true". > > Since you're chasing a suspected bug in even mentioning AEC, I suggest you > transform this to: > if (!Build.MODEL.equals("SM-T310R") && > !Build.MODEL.equals("GT-I9300") ... > !Build.MODEL.equals("Nexus 7")) { > return false; > } > return AcousticEchoCanceler.isAvailable(); gaa! thanks Ami, I should have caught that. This was done correctly in a much earlier patch set.
https://codereview.chromium.org/156353003/diff/60001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/60001/media/base/android/java/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:444: AcousticEchoCanceler.isAvailable()); see here.
Message was sent while issue was closed.
Change committed as 249756
D'oh! I expected my comment to abort the CQ run but it didn't. Reverted in 249767. On Fri, Feb 7, 2014 at 12:24 PM, <commit-bot@chromium.org> wrote: > Change committed as 249756 > > https://codereview.chromium.org/156353003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Thanks Ami. Will take all you comments into consideration on Monday.
Message was sent while issue was closed.
Did changes based on great review from Ami. Many thanks for finding the bug that I committed. https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:207: } catch (Exception e) { On 2014/02/07 17:06:45, Ami Fischman wrote: > In general while coding Java (say, when not trying to poke in the dark at an > unreproducible bug) "catch (Exception" is a sure sign of something being wrong. > E.g. this will black-hole OOM, JVM bugs, etc. > This deserves a comment + TODO/crbug to remove once we have data about whether > it helps. > > (ditto for elsewhere in the file where you make changes that would not be > acceptable in the normal course of things). Done. https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:209: logwtf("SettingsObserver exception: " + e.getMessage()); Thanks. Remove bad helper in this CL. https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:212: mIsInitialized = true; FYI, Was discussed earlier in this review. Returning true is fine (best I can do here) since it is fair to say that we are initialized even if try triggers. It "only" means that one isolated part (detect volume changes) is not working as it should but the main functionality is till active. https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:230: } catch (Exception e) { Good point. Reverted. https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:238: } catch (Exception e) { Good point. Reverted. https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:440: On 2014/02/07 17:06:45, Ami Fischman wrote: > extra \n Done. https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:456: Build.MODEL.equals("SM-N9005") || // Galaxy Note 3 Will have to refer to ajm@ who has done these parts. Andrew: any comments? https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:459: Build.MODEL.equals("Nexus 7") && Many thanks Ami! My bad. https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:566: btAdapter = null; On 2014/02/07 17:06:45, Ami Fischman wrote: > log? Done. https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:574: btAdapter = null; On 2014/02/07 17:06:45, Ami Fischman wrote: > log? Done. https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:579: return false; On 2014/02/07 17:06:45, Ami Fischman wrote: > Then why not instead simply return false from the catch blocks above? Done. https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:582: android.bluetooth.BluetoothProfile.STATE_DISCONNECTED; On 2014/02/07 17:06:45, Ami Fischman wrote: > this assignment is useless? Done. https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:591: return (btAdapter.isEnabled() && profileConnectionState == On 2014/02/07 17:06:45, Ami Fischman wrote: > isEnabled() is new in this change. Are you worried that an adapter may be > CONNECTED but disabled? > (worth a comment) Done.
Message was sent while issue was closed.
https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/156353003/diff/580001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:456: Build.MODEL.equals("SM-N9005") || // Galaxy Note 3 On 2014/02/10 11:14:10, henrika wrote: > Will have to refer to ajm@ who has done these parts. > > Andrew: any comments? I'd prefer it to be sorted by model number. |
