Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
874683006:
Chromecast: makes CastWindowAndroid use CastContentWindow. (Closed)
DescriptionChromecast: makes CastWindowAndroid use CastContentWindow.
This unifies the WebContents creation code between each platform.
R=lcwu@chromium.org,byungchul@chromium.org
BUG=internal b/18934906
Committed: https://crrev.com/cd38a84418d8d192199016af8c3902e6fea61d5a
Cr-Commit-Position: refs/heads/master@{#315818}
Patch Set 1 #
Total comments: 4
Patch Set 2 : better naming. removed test hack. #Patch Set 3 : adds explicit #
Total comments: 2
Patch Set 4 : change ScopedJavaLocalRef behavior #
Messages
Total messages: 17 (4 generated)
https://codereview.chromium.org/874683006/diff/1/chromecast/browser/android/c... File chromecast/browser/android/cast_window_android.cc (right): https://codereview.chromium.org/874683006/diff/1/chromecast/browser/android/c... chromecast/browser/android/cast_window_android.cc:48: window_(new CastContentWindow), Given that you renamed 'shell' to 'window' for CastWindowAndroid in the function above, having a data member named 'window_' which is actually a CastContentWindow is a bit confusing. Maybe we should change the data member name to 'content_window_'? https://codereview.chromium.org/874683006/diff/1/chromecast/browser/android/c... File chromecast/browser/android/cast_window_android.h (right): https://codereview.chromium.org/874683006/diff/1/chromecast/browser/android/c... chromecast/browser/android/cast_window_android.h:80: CastWindowAndroid(content::BrowserContext* browser_context); Add 'explicit'.
https://codereview.chromium.org/874683006/diff/1/chromecast/browser/android/c... File chromecast/browser/android/cast_window_android.cc (right): https://codereview.chromium.org/874683006/diff/1/chromecast/browser/android/c... chromecast/browser/android/cast_window_android.cc:48: window_(new CastContentWindow), On 2015/02/04 00:28:31, lcwu1 wrote: > Given that you renamed 'shell' to 'window' for CastWindowAndroid in the function > above, having a data member named 'window_' which is actually a > CastContentWindow is a bit confusing. Maybe we should change the data member > name to 'content_window_'? Oof, the naming did get rough. Renamed a couple things to try to make this clearer. https://codereview.chromium.org/874683006/diff/1/chromecast/browser/android/c... File chromecast/browser/android/cast_window_android.h (right): https://codereview.chromium.org/874683006/diff/1/chromecast/browser/android/c... chromecast/browser/android/cast_window_android.h:80: CastWindowAndroid(content::BrowserContext* browser_context); On 2015/02/04 00:28:31, lcwu1 wrote: > Add 'explicit'. Done.
lgtm
The CQ bit was checked by gunsch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/874683006/40001
https://codereview.chromium.org/874683006/diff/40001/chromecast/browser/andro... File chromecast/browser/android/cast_window_android.cc (right): https://codereview.chromium.org/874683006/diff/40001/chromecast/browser/andro... chromecast/browser/android/cast_window_android.cc:59: window_java_.Reset(env, CreateCastWindowView(this).Release()); I think it makes a memory leak since CreateCastWindowView() returns a local reference while window_java is a global ref. Release() doesn't delete local reference. I think just removing Release() would work, i.e window_java_.Reset() gets new global ref, and destruction of return value of CreateCastWindowView(this) destroy local ref.
The CQ bit was unchecked by gunsch@chromium.org
https://codereview.chromium.org/874683006/diff/40001/chromecast/browser/andro... File chromecast/browser/android/cast_window_android.cc (right): https://codereview.chromium.org/874683006/diff/40001/chromecast/browser/andro... chromecast/browser/android/cast_window_android.cc:59: window_java_.Reset(env, CreateCastWindowView(this).Release()); On 2015/02/04 01:04:52, byungchul wrote: > I think it makes a memory leak since CreateCastWindowView() returns a local > reference while window_java is a global ref. Release() doesn't delete local > reference. I think just removing Release() would work, i.e window_java_.Reset() > gets new global ref, and destruction of return value of > CreateCastWindowView(this) destroy local ref. Removing |Release()| call doesn't compile, because of API: |ScopedJavaLocalRef| won't implicitly release the reference. |CreateCastWindowView()| returns a |ScopedJavaLocalRef|, which isn't compatible with |ScopedJavaGlobalRef.Reset(JNIEnv*, T)|. I'm not sure what memory leak you're talking about though: the |ScopedJavaLocalRef| object itself is stack-allocated, and calling |Release()| gives us the underlying Java reference, for which we take ownership in a |ScopedJavaGlobalRef| that's tied to the lifetime of CastWindowAndroid. Am I missing something?
On 2015/02/04 01:16:32, gunsch wrote: > https://codereview.chromium.org/874683006/diff/40001/chromecast/browser/andro... > File chromecast/browser/android/cast_window_android.cc (right): > > https://codereview.chromium.org/874683006/diff/40001/chromecast/browser/andro... > chromecast/browser/android/cast_window_android.cc:59: window_java_.Reset(env, > CreateCastWindowView(this).Release()); > On 2015/02/04 01:04:52, byungchul wrote: > > I think it makes a memory leak since CreateCastWindowView() returns a local > > reference while window_java is a global ref. Release() doesn't delete local > > reference. I think just removing Release() would work, i.e > window_java_.Reset() > > gets new global ref, and destruction of return value of > > CreateCastWindowView(this) destroy local ref. > > Removing |Release()| call doesn't compile, because of API: |ScopedJavaLocalRef| > won't implicitly release the reference. > > |CreateCastWindowView()| returns a |ScopedJavaLocalRef|, which isn't compatible > with |ScopedJavaGlobalRef.Reset(JNIEnv*, T)|. > > I'm not sure what memory leak you're talking about though: the > |ScopedJavaLocalRef| object itself is stack-allocated, and calling |Release()| > gives us the underlying Java reference, for which we take ownership in a > |ScopedJavaGlobalRef| that's tied to the lifetime of CastWindowAndroid. > > Am I missing something? ScopedJavaLocalRef calls NewLocalRef() in ctor (or Reset) and DeleteLocalRef() in dtor. Release() causes skipping DeleteLocalRef() which leads the java object not released until JNI functions return to java side. In practice, it might not be a problem, but if this function is called again and again, or the call path never returns to java side, it is a memory leak.
New patchsets have been uploaded after l-g-t-m from lcwu@chromium.org
On 2015/02/04 01:28:10, byungchul wrote: > On 2015/02/04 01:16:32, gunsch wrote: > > > https://codereview.chromium.org/874683006/diff/40001/chromecast/browser/andro... > > File chromecast/browser/android/cast_window_android.cc (right): > > > > > https://codereview.chromium.org/874683006/diff/40001/chromecast/browser/andro... > > chromecast/browser/android/cast_window_android.cc:59: window_java_.Reset(env, > > CreateCastWindowView(this).Release()); > > On 2015/02/04 01:04:52, byungchul wrote: > > > I think it makes a memory leak since CreateCastWindowView() returns a local > > > reference while window_java is a global ref. Release() doesn't delete local > > > reference. I think just removing Release() would work, i.e > > window_java_.Reset() > > > gets new global ref, and destruction of return value of > > > CreateCastWindowView(this) destroy local ref. > > > > Removing |Release()| call doesn't compile, because of API: > |ScopedJavaLocalRef| > > won't implicitly release the reference. > > > > |CreateCastWindowView()| returns a |ScopedJavaLocalRef|, which isn't > compatible > > with |ScopedJavaGlobalRef.Reset(JNIEnv*, T)|. > > > > I'm not sure what memory leak you're talking about though: the > > |ScopedJavaLocalRef| object itself is stack-allocated, and calling |Release()| > > gives us the underlying Java reference, for which we take ownership in a > > |ScopedJavaGlobalRef| that's tied to the lifetime of CastWindowAndroid. > > > > Am I missing something? > > ScopedJavaLocalRef calls NewLocalRef() in ctor (or Reset) and DeleteLocalRef() > in dtor. Release() causes skipping DeleteLocalRef() which leads the java object > not released until JNI functions return to java side. In practice, it might not > be a problem, but if this function is called again and again, or the call path > never returns to java side, it is a memory leak. Ah yes, now we have a NewLocalRef/DeleteLocalRef pair, but also a NewGlobalRef/DeleteGlobalRef pair. Agree that this is better, probably was safe already since Java would call into JNI and have a quick return in this case, but good to do anyways.
lgtm
The CQ bit was checked by gunsch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/874683006/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/cd38a84418d8d192199016af8c3902e6fea61d5a Cr-Commit-Position: refs/heads/master@{#315818} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
