Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11823027:
[Android WebView] Implement the capture picture API. (Closed)
|
Created:
7 years, 11 months ago by Leandro Graciá Gil Modified:
7 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, android-webview-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Android WebView] Implement the capture picture API.
BUG=167908
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179693
Patch Set 1 #Patch Set 2 : adding findbugs update from 11825002 to make the trybots happy. #
Total comments: 9
Patch Set 3 : new approach without ExternalPictureData. #
Total comments: 16
Patch Set 4 : review fixes. #Patch Set 5 : rebase #
Total comments: 8
Patch Set 6 : nit fixes. #Patch Set 7 : re-uploading to try cq again. #Patch Set 8 : fix WebView instrumentation test crashes. #Patch Set 9 : fixed synchronous IPC problems that crashed/deadlocked tests. #
Total comments: 6
Patch Set 10 : ensure synchronous IPC replies to prevent deadlocks. #Patch Set 11 : changes in native/Java AwContents to support SW rendering without platform functions (tests). #
Total comments: 8
Patch Set 12 : review fixes + added checks for empty views. #Patch Set 13 : workaround IPC issue, dchecking but replying in release mode. Fixing rendering for empty views. #Patch Set 14 : limit sync replies to non-handled messages. #Patch Set 15 : removing any IPC DCHECKS. #Messages
Total messages: 52 (0 generated)
joth: android_webview code. joi: render_view_observer.h. See comment in file for details of why. Thanks! https://codereview.chromium.org/11823027/diff/4001/content/public/renderer/re... File content/public/renderer/render_view_observer.h (right): https://codereview.chromium.org/11823027/diff/4001/content/public/renderer/re... content/public/renderer/render_view_observer.h:102: // IPC::Sender implementation. If not moved to public, the IPC_MESSAGE_HANDLER_DELAY_REPLY macro in aw_render_view_ext.cc throws the following build error: In file included from android_webview/renderer/aw_render_view_ext.h:12:0, from android_webview/renderer/aw_render_view_ext.cc:5: ipc/ipc_message_utils.h: In instantiation of 'static bool IPC::SyncMessageSchema<SendParamType, ReplyParamType>::DispatchDelayReplyWithSendParams(bool, const SendParam&, const IPC::Message*, T*, Method) [with T = android_webview::AwRenderViewExt; Method = void (android_webview::AwRenderViewExt::*)(IPC::Message*); SendParamType = Tuple0; ReplyParamType = Tuple0; IPC::SyncMessageSchema<SendParamType, ReplyParamType>::SendParam = Tuple0]': android_webview/common/render_view_messages.h:57:1: required from 'static bool AwViewMsg_CapturePictureSync::DispatchDelayReply(const IPC::Message*, T*, Method) [with T = android_webview::AwRenderViewExt; Method = void (android_webview::AwRenderViewExt::*)(IPC::Message*); IPC::Message = IPC::Message]' android_webview/renderer/aw_render_view_ext.cc:71:5: required from here content/public/renderer/render_view_observer.h:107:16: error: 'virtual bool content::RenderViewObserver::Send(IPC::Message*)' is protected In file included from ipc/ipc_message_macros.h:188:0, from content/public/common/common_param_traits_macros.h:16, from content/public/common/common_param_traits.h:19, from android_webview/common/render_view_messages.h:7, from android_webview/renderer/aw_render_view_ext.cc:10: ipc/ipc_message_utils.h:831:7: error: within this context I'm open to alternative ways to solve this.
palmer: new IPC message in android_webview/common/render_view_messages.h. This one synchronously requests a SkPicture with the latest renderer compositor contents in order to support the WebView.capturePicture API.
https://codereview.chromium.org/11823027/diff/4001/android_webview/common/ren... File android_webview/common/render_view_messages.h (right): https://codereview.chromium.org/11823027/diff/4001/android_webview/common/ren... android_webview/common/render_view_messages.h:58: IPC_SYNC_MESSAGE_ROUTED0_0(AwViewMsg_CapturePictureSync) I'll add a description here in the next iteration of the patch.
https://codereview.chromium.org/11823027/diff/4001/content/public/renderer/re... File content/public/renderer/render_view_observer.h (right): https://codereview.chromium.org/11823027/diff/4001/content/public/renderer/re... content/public/renderer/render_view_observer.h:102: // IPC::Sender implementation. On 2013/01/09 16:23:00, Leandro Graciá Gil wrote: > If not moved to public, the IPC_MESSAGE_HANDLER_DELAY_REPLY macro in > aw_render_view_ext.cc throws the following build error: > > In file included from android_webview/renderer/aw_render_view_ext.h:12:0, > from android_webview/renderer/aw_render_view_ext.cc:5: > ipc/ipc_message_utils.h: In instantiation of 'static bool > IPC::SyncMessageSchema<SendParamType, > ReplyParamType>::DispatchDelayReplyWithSendParams(bool, const SendParam&, const > IPC::Message*, T*, Method) [with T = android_webview::AwRenderViewExt; Method = > void (android_webview::AwRenderViewExt::*)(IPC::Message*); SendParamType = > Tuple0; ReplyParamType = Tuple0; IPC::SyncMessageSchema<SendParamType, > ReplyParamType>::SendParam = Tuple0]': > android_webview/common/render_view_messages.h:57:1: required from 'static bool > AwViewMsg_CapturePictureSync::DispatchDelayReply(const IPC::Message*, T*, > Method) [with T = android_webview::AwRenderViewExt; Method = void > (android_webview::AwRenderViewExt::*)(IPC::Message*); IPC::Message = > IPC::Message]' > android_webview/renderer/aw_render_view_ext.cc:71:5: required from here > content/public/renderer/render_view_observer.h:107:16: error: 'virtual bool > content::RenderViewObserver::Send(IPC::Message*)' is protected > In file included from ipc/ipc_message_macros.h:188:0, > from content/public/common/common_param_traits_macros.h:16, > from content/public/common/common_param_traits.h:19, > from android_webview/common/render_view_messages.h:7, > from android_webview/renderer/aw_render_view_ext.cc:10: > ipc/ipc_message_utils.h:831:7: error: within this context > > I'm open to alternative ways to solve this. Hmm, I don't know if we want to make this public on the class in general, although that might be OK. Could you try making it public only in your subclass, i.e. put something like this in the public part of your class: using content::RenderViewObserver::Send; If this works, please include a comment explaining why this is needed.
didn't read it all yet, just skimming https://codereview.chromium.org/11823027/diff/4001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/11823027/diff/4001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:96: private final AwContentsClient.ExternalPictureData mExternalPictureData; rather than final I'd be OK lazy initializing this. (it shouldn't be needed by majority of users, and would only be accessed on main thread) https://codereview.chromium.org/11823027/diff/4001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:864: mExternalPictureData.height = height; you shouldn't need the find bugs suppression as you're assigning all the fields.
https://codereview.chromium.org/11823027/diff/4001/android_webview/common/ren... File android_webview/common/render_view_messages.h (right): https://codereview.chromium.org/11823027/diff/4001/android_webview/common/ren... android_webview/common/render_view_messages.h:58: IPC_SYNC_MESSAGE_ROUTED0_0(AwViewMsg_CapturePictureSync) On 2013/01/09 16:30:13, Leandro Graciá Gil wrote: > I'll add a description here in the next iteration of the patch. Done. https://codereview.chromium.org/11823027/diff/4001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/11823027/diff/4001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:96: private final AwContentsClient.ExternalPictureData mExternalPictureData; Not required any longer. On 2013/01/10 01:45:39, joth wrote: > rather than final I'd be OK lazy initializing this. (it shouldn't be needed by > majority of users, and would only be accessed on main thread) https://codereview.chromium.org/11823027/diff/4001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:864: mExternalPictureData.height = height; Actually, this was replacing the old bugs with new ones as they are publicly accessible by anyone. In any case this is not required anymore. On 2013/01/10 01:45:39, joth wrote: > you shouldn't need the find bugs suppression as you're assigning all the fields. https://codereview.chromium.org/11823027/diff/4001/content/public/renderer/re... File content/public/renderer/render_view_observer.h (right): https://codereview.chromium.org/11823027/diff/4001/content/public/renderer/re... content/public/renderer/render_view_observer.h:102: // IPC::Sender implementation. That worked, thanks! No changes required to this file. On 2013/01/09 19:23:45, Jói wrote: > On 2013/01/09 16:23:00, Leandro Graciá Gil wrote: > > If not moved to public, the IPC_MESSAGE_HANDLER_DELAY_REPLY macro in > > aw_render_view_ext.cc throws the following build error: > > > > In file included from android_webview/renderer/aw_render_view_ext.h:12:0, > > from android_webview/renderer/aw_render_view_ext.cc:5: > > ipc/ipc_message_utils.h: In instantiation of 'static bool > > IPC::SyncMessageSchema<SendParamType, > > ReplyParamType>::DispatchDelayReplyWithSendParams(bool, const SendParam&, > const > > IPC::Message*, T*, Method) [with T = android_webview::AwRenderViewExt; Method > = > > void (android_webview::AwRenderViewExt::*)(IPC::Message*); SendParamType = > > Tuple0; ReplyParamType = Tuple0; IPC::SyncMessageSchema<SendParamType, > > ReplyParamType>::SendParam = Tuple0]': > > android_webview/common/render_view_messages.h:57:1: required from 'static > bool > > AwViewMsg_CapturePictureSync::DispatchDelayReply(const IPC::Message*, T*, > > Method) [with T = android_webview::AwRenderViewExt; Method = void > > (android_webview::AwRenderViewExt::*)(IPC::Message*); IPC::Message = > > IPC::Message]' > > android_webview/renderer/aw_render_view_ext.cc:71:5: required from here > > content/public/renderer/render_view_observer.h:107:16: error: 'virtual bool > > content::RenderViewObserver::Send(IPC::Message*)' is protected > > In file included from ipc/ipc_message_macros.h:188:0, > > from content/public/common/common_param_traits_macros.h:16, > > from content/public/common/common_param_traits.h:19, > > from android_webview/common/render_view_messages.h:7, > > from android_webview/renderer/aw_render_view_ext.cc:10: > > ipc/ipc_message_utils.h:831:7: error: within this context > > > > I'm open to alternative ways to solve this. > > Hmm, I don't know if we want to make this public on the class in general, > although that might be OK. > > Could you try making it public only in your subclass, i.e. put something like > this in the public part of your class: > > using content::RenderViewObserver::Send; > > If this works, please include a comment explaining why this is needed.
https://codereview.chromium.org/11823027/diff/11001/android_webview/native/aw... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/11823027/diff/11001/android_webview/native/aw... android_webview/native/aw_contents.cc:470: g_is_skia_version_compatible = This needs to be temporarily commented out until the glue layer provides this method in order to prevent crashes on startup.
Wow. one gnarly patch. Structure is totally good to me now, only comment comments now I think. - lgtm when you're happy with it. https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/br... File android_webview/browser/renderer_host/aw_render_view_host_ext.h (right): https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/br... android_webview/browser/renderer_host/aw_render_view_host_ext.h:53: // Enables receiving picture piles on every new frame. nit: receiving -> updating? (receiving made me look for a callback param). also mention where the result is put. (and on method below). https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/ja... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/ja... android_webview/java/src/org/chromium/android_webview/AwContents.java:984: Bitmap bitmap = Bitmap.createBitmap(width, height, Bitmap.Config.ARGB_8888); maybe add a comment for the obvious: we could cache the bitmap to slightly reduce cost of this approach if necessary (e.g. put it in a static SoftReference<>). (but I agree with your comment above that this isn't worth it now). https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/na... File android_webview/native/aw_contents.cc (right): https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/na... android_webview/native/aw_contents.cc:711: if (on_new_picture_enabled_ && on_new_picture_invalidation_only_) comment where we do the callback if it's !invalidation only. https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/na... android_webview/native/aw_contents.cc:929: on_new_picture_invalidation_only_ = invalidation_only; I'm wondering if these two would be better as an enum { Off, On, InvalidationOnly}. https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/na... File android_webview/native/aw_contents.h (right): https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/na... android_webview/native/aw_contents.h:145: // If none is available will synchronously request the latest one. .. and block until the result is received. (I know that's what synchronously means, but worth being explicit). https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/pu... File android_webview/public/browser/draw_sw.h (right): https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/pu... android_webview/public/browser/draw_sw.h:43: typedef void (SkiaVersionFunction)(int* major, int* minor, int* patch); we probably don't need this one now? this you just call the method below (which sgtm) https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/re... File android_webview/renderer/aw_render_view_ext.cc (right): https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/re... android_webview/renderer/aw_render_view_ext.cc:52: // TODO(leandrogracia): remove when SW rendering uses Ubercompositor. Haha It was bad enough as a TODO to enable the code, but really, a TODO to remove some not yet enabled code? :-D ;-) But yeah I think this is best we can do for now..
https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/br... File android_webview/browser/renderer_host/aw_render_view_host_ext.h (right): https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/br... android_webview/browser/renderer_host/aw_render_view_host_ext.h:53: // Enables receiving picture piles on every new frame. On 2013/01/12 02:36:28, joth wrote: > nit: receiving -> updating? (receiving made me look for a callback param). > > also mention where the result is put. (and on method below). > Done. https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/ja... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/ja... android_webview/java/src/org/chromium/android_webview/AwContents.java:984: Bitmap bitmap = Bitmap.createBitmap(width, height, Bitmap.Config.ARGB_8888); On 2013/01/12 02:36:28, joth wrote: > maybe add a comment for the obvious: we could cache the bitmap to slightly > reduce cost of this approach if necessary (e.g. put it in a static > SoftReference<>). > > (but I agree with your comment above that this isn't worth it now). Interesting, I didn't know about soft references. Done. https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/na... File android_webview/native/aw_contents.cc (right): https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/na... android_webview/native/aw_contents.cc:470: g_is_skia_version_compatible = On 2013/01/10 19:52:47, Leandro Graciá Gil wrote: > This needs to be temporarily commented out until the glue layer provides this > method in order to prevent crashes on startup. Done. https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/na... android_webview/native/aw_contents.cc:711: if (on_new_picture_enabled_ && on_new_picture_invalidation_only_) On 2013/01/12 02:36:28, joth wrote: > comment where we do the callback if it's !invalidation only. Done. https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/na... android_webview/native/aw_contents.cc:929: on_new_picture_invalidation_only_ = invalidation_only; On 2013/01/12 02:36:28, joth wrote: > I'm wondering if these two would be better as an enum { Off, On, > InvalidationOnly}. Done. https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/na... File android_webview/native/aw_contents.h (right): https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/na... android_webview/native/aw_contents.h:145: // If none is available will synchronously request the latest one. On 2013/01/12 02:36:28, joth wrote: > .. and block until the result is received. > (I know that's what synchronously means, but worth being explicit). Done. https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/pu... File android_webview/public/browser/draw_sw.h (right): https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/pu... android_webview/public/browser/draw_sw.h:43: typedef void (SkiaVersionFunction)(int* major, int* minor, int* patch); On 2013/01/12 02:36:28, joth wrote: > we probably don't need this one now? this you just call the method below (which > sgtm) That's the type of the argument of the method below (which should be SkGraphics::GetVersion). https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/re... File android_webview/renderer/aw_render_view_ext.cc (right): https://chromiumcodereview.appspot.com/11823027/diff/11001/android_webview/re... android_webview/renderer/aw_render_view_ext.cc:52: // TODO(leandrogracia): remove when SW rendering uses Ubercompositor. On 2013/01/12 02:36:28, joth wrote: > Haha It was bad enough as a TODO to enable the code, but really, a TODO to > remove some not yet enabled code? :-D ;-) > But yeah I think this is best we can do for now.. First TODO get SW rendering work using CapturePicture once the API is ready in RenderView. Second TODO to remove this in favor of the real Ubercompositor SW rendering when we can. Yes, just the TODOs look a bit silly...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/11823027/22001
Presubmit check for 11823027-22001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
android_webview/common/render_view_messages.h
Presubmit checks took 1.5s to calculate.
lgtm https://chromiumcodereview.appspot.com/11823027/diff/22001/android_webview/ja... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://chromiumcodereview.appspot.com/11823027/diff/22001/android_webview/ja... android_webview/java/src/org/chromium/android_webview/AwContents.java:1013: mPictureBitmapCache.get().getHeight() != height) { nit: this is technically racy, as the ref maybe GCed between the 2nd & third clause. IMO cleanest pattern is: /// avoid the initial null check... SoftReference<Bitmap> mPictureBitmapCache = new SoftReference<Bitmap>(); ... Bitmap bitmap = mPictureBitmapCache.get(); if (bitmap == null || bitmap.width != w || bitmap.h != h) { bitmap = Bitmap.create(...) mPictureBitmapCache.set(bitmap); } ...
ping palmer for aw/common IPC review.
Looking at it now. (I was out of the office at a conference last week. Sorry for the delay!)
LGTM from IPC security POV. https://codereview.chromium.org/11823027/diff/22001/android_webview/native/aw... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/11823027/diff/22001/android_webview/native/aw... android_webview/native/aw_contents.cc:384: // TODO(leandrogracia): once Ubercompositor is ready and we support software I'd file a bug and refer to it here. https://codereview.chromium.org/11823027/diff/22001/android_webview/native/aw... android_webview/native/aw_contents.cc:469: // TODO(leandrogracia): uncomment once the glue layer implements this method. Here, too. https://codereview.chromium.org/11823027/diff/22001/android_webview/renderer/... File android_webview/renderer/aw_render_view_ext.cc (right): https://codereview.chromium.org/11823027/diff/22001/android_webview/renderer/... android_webview/renderer/aw_render_view_ext.cc:52: // TODO(leandrogracia): remove when SW rendering uses Ubercompositor. Again, it's my preference to tie TODOs to live bugs.
https://codereview.chromium.org/11823027/diff/22001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/11823027/diff/22001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:1013: mPictureBitmapCache.get().getHeight() != height) { On 2013/01/14 23:20:57, joth wrote: > nit: this is technically racy, as the ref maybe GCed between the 2nd & third > clause. > IMO cleanest pattern is: > > /// avoid the initial null check... > SoftReference<Bitmap> mPictureBitmapCache = new SoftReference<Bitmap>(); > ... > > > Bitmap bitmap = mPictureBitmapCache.get(); > if (bitmap == null || bitmap.width != w || bitmap.h != h) { > bitmap = Bitmap.create(...) > mPictureBitmapCache.set(bitmap); > } > ... Two issues with your approach: 1. SoftReference<> doesn't have a default constructor, so we need to create it with the actual reference to keep. 2. SoftReference<> doesn't have a set method (goto 1). I'll be fixing the racy .get(). https://codereview.chromium.org/11823027/diff/22001/android_webview/native/aw... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/11823027/diff/22001/android_webview/native/aw... android_webview/native/aw_contents.cc:384: // TODO(leandrogracia): once Ubercompositor is ready and we support software On 2013/01/15 01:16:19, Chris P. wrote: > I'd file a bug and refer to it here. Done. https://codereview.chromium.org/11823027/diff/22001/android_webview/native/aw... android_webview/native/aw_contents.cc:469: // TODO(leandrogracia): uncomment once the glue layer implements this method. On 2013/01/15 01:16:19, Chris P. wrote: > Here, too. This one is intended to be very shortly-lived, probably gone by the end of this week. We depend on another patch for build to work, and therefore we need to temporarily comment a few bits of code to prevent breaking the build or blocking ourselves. That's why these have not a bug filed. https://codereview.chromium.org/11823027/diff/22001/android_webview/renderer/... File android_webview/renderer/aw_render_view_ext.cc (right): https://codereview.chromium.org/11823027/diff/22001/android_webview/renderer/... android_webview/renderer/aw_render_view_ext.cc:52: // TODO(leandrogracia): remove when SW rendering uses Ubercompositor. On 2013/01/15 01:16:19, Chris P. wrote: > Again, it's my preference to tie TODOs to live bugs. Adding crbug link. In these cases we want to leave clear pointers in the code of the parts that will need to be changed once Ubercompositor is ready, and have as few of them as possible.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/11823027/29001
Failed to request the patch to try. Please note that binary files are still unsupported at the moment, this is being worked on. Thanks for your patience. HTTP Error 500: <html><head> <meta http-equiv="content-type" content="text/html;charset=utf-8"> <title>500 Server Error</title> </head> <body text=#000000 bgcolor=#ffffff> <h1>Error: Server Error</h1> <h2>The server encountered an error and could not complete your request.<p>If the problem persists, please <A HREF="http://code.google.com/appengine/community.html">report</A> your problem and mention this error message and the query that caused it.</h2> <h2></h2> </body></html>
Failed to request the patch to try. Please note that binary files are still unsupported at the moment, this is being worked on. Thanks for your patience. HTTP Error 500: <html><head> <meta http-equiv="content-type" content="text/html;charset=utf-8"> <title>500 Server Error</title> </head> <body text=#000000 bgcolor=#ffffff> <h1>Error: Server Error</h1> <h2>The server encountered an error and could not complete your request.<p>If the problem persists, please <A HREF="http://code.google.com/appengine/community.html">report</A> your problem and mention this error message and the query that caused it.</h2> <h2></h2> </body></html>
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/11823027/29001
Failed to trigger a try job on android_clang_dbg HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/11823027/35001
Presubmit check for 11823027-35001 failed and returned exit status 1.
Running presubmit commit checks ...
Traceback (most recent call last):
File "/b/commit-queue/verification/presubmit_shim.py", line 43, in <module>
sys.exit(presubmit_support.Main(argv))
File "/b/depot_tools/presubmit_support.py", line 1259, in Main
rietveld_obj)
File "/b/depot_tools/presubmit_support.py", line 1107, in DoPresubmitChecks
results += executer.ExecPresubmitScript(presubmit_script, filename)
File "/b/depot_tools/presubmit_support.py", line 1024, in ExecPresubmitScript
result = eval(function_name + '(*__args)', context)
File "<string>", line 1, in <module>
File "<string>", line 798, in CheckChangeOnCommit
File "<string>", line 680, in _CommonChecks
File "/b/depot_tools/presubmit_canned_checks.py", line 966, in
PanProjectChecks
input_api, output_api, source_file_filter=None))
File "/b/depot_tools/presubmit_canned_checks.py", line 784, in CheckOwners
approval_needed=input_api.is_committing)
File "/b/depot_tools/presubmit_canned_checks.py", line 826, in
_RietveldOwnerAndReviewers
issue_props = _GetRietveldIssueProps(input_api, True)
File "/b/depot_tools/presubmit_canned_checks.py", line 817, in
_GetRietveldIssueProps
issue=int(issue), messages=messages)
File "/b/depot_tools/rietveld.py", line 439, in get_issue_properties
super(CachingRietveld, self).get_issue_properties)
File "/b/depot_tools/rietveld.py", line 418, in _lookup
function_cache[args] = update(*args)
File "/b/depot_tools/rietveld.py", line 93, in get_issue_properties
return json.loads(self.get(url))
File "/b/depot_tools/rietveld.py", line 352, in get
return self._send(request_path, **kwargs)
File "/b/depot_tools/rietveld.py", line 378, in _send
result = self.rpc_server.Send(request_path, **kwargs)
File "/b/depot_tools/third_party/upload.py", line 416, in Send
ErrorExit(e.read())
File "/b/depot_tools/rietveld.py", line 370, in trap_http_500
raise urllib2.HTTPError(request_path, m.group(1), msg, None, None)
urllib2.HTTPError: HTTP Error 500:
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/11823027/35001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/11823027/55001
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is android_dbg, revision is HEAD
jam: just before landing this patch we found some corner cases in WebView tests where legacy APIs triggering synchronous browser -> renderer IPCs were sent before the RenderView has been created. This caused problems like crashes and deadlocks. Looking further for the cause I found 2 IPC issues: 1. When a synchronous message is not handled (e.g. no RenderView yet) we don't ever send a reply back, causing a deadlock. 2. There are some cases in RenderProcessHostImpl where we enqueue the messages if we're not ready to send them to try again later. That's ok for asynchronous messages, but synchronous ones should simply fail and return false. Could you please take a look to the changes in render_process_host_impl.cc and ipc_channel_proxy.cc? Thanks.
https://codereview.chromium.org/11823027/diff/47002/ipc/ipc_channel_proxy.cc File ipc/ipc_channel_proxy.cc (right): https://codereview.chromium.org/11823027/diff/47002/ipc/ipc_channel_proxy.cc#... ipc/ipc_channel_proxy.cc:264: if (!handled && message.is_sync()) { Should this also happen in other places returning in this method? (e.g. if there is no listener)
https://codereview.chromium.org/11823027/diff/47002/ipc/ipc_channel_proxy.cc File ipc/ipc_channel_proxy.cc (right): https://codereview.chromium.org/11823027/diff/47002/ipc/ipc_channel_proxy.cc#... ipc/ipc_channel_proxy.cc:263: // Prevent non-handled synchronous messages to deadlock waiting for a reply. uber-nit: this is not strictly always going to result in deadlock, it's a straight hang (waiting for an event that is not going to happen). https://codereview.chromium.org/11823027/diff/47002/ipc/ipc_channel_proxy.cc#... ipc/ipc_channel_proxy.cc:271: } this seems pretty reasonable to me, FWIW, but I'd recommend landing it first in its own patch? splitting the android-only parts from this common code would help keep the two apart as they cycle through the bots.
On 2013/01/18 00:31:54, Leandro Graciá Gil wrote: > jam: just before landing this patch we found some corner cases in WebView tests > where legacy APIs triggering synchronous browser -> renderer IPCs were sent > before the RenderView has been created. This caused problems like crashes and > deadlocks. Looking further for the cause I found 2 IPC issues: > > 1. When a synchronous message is not handled (e.g. no RenderView yet) we don't > ever send a reply back, causing a deadlock. > > 2. There are some cases in RenderProcessHostImpl where we enqueue the messages > if we're not ready to send them to try again later. That's ok for asynchronous > messages, but synchronous ones should simply fail and return false. > > Could you please take a look to the changes in render_process_host_impl.cc and > ipc_channel_proxy.cc? Thanks. i'm wary of adding complexity to shared code to support the legacy webview case. can you fix this by other ways, i.e. where you send the sync IPCs for the legacy IPCs, check if a RVH is live or not?
On 2013/01/18 18:37:29, John Abd-El-Malek wrote: > On 2013/01/18 00:31:54, Leandro Graciá Gil wrote: > > jam: just before landing this patch we found some corner cases in WebView > tests > > where legacy APIs triggering synchronous browser -> renderer IPCs were sent > > before the RenderView has been created. This caused problems like crashes and > > deadlocks. Looking further for the cause I found 2 IPC issues: > > > > 1. When a synchronous message is not handled (e.g. no RenderView yet) we don't > > ever send a reply back, causing a deadlock. > > > > 2. There are some cases in RenderProcessHostImpl where we enqueue the messages > > if we're not ready to send them to try again later. That's ok for asynchronous > > messages, but synchronous ones should simply fail and return false. > > > > Could you please take a look to the changes in render_process_host_impl.cc and > > ipc_channel_proxy.cc? Thanks. > > i'm wary of adding complexity to shared code to support the legacy webview case. > > can you fix this by other ways, i.e. where you send the sync IPCs for the legacy > IPCs, check if a RVH is live or not? We can try to workaround this, but that won't solve the real problem and might lead to further and unexpected deadlocks in the future. We know there are 2 real issues in sync IPCs and only one of them should be limited to the Browser -> Renderer case (which happens to be WebView-only for now, but it's not necessarily "for WebView"). We can't guarantee either us or somebody else will bump into them in the future in some other way. I'm open to any suggestions you might have to reduce complexity, but I do think we should fix the real problem rather than trying to work around it.
https://codereview.chromium.org/11823027/diff/47002/ipc/ipc_channel_proxy.cc File ipc/ipc_channel_proxy.cc (right): https://codereview.chromium.org/11823027/diff/47002/ipc/ipc_channel_proxy.cc#... ipc/ipc_channel_proxy.cc:263: // Prevent non-handled synchronous messages to deadlock waiting for a reply. On 2013/01/18 01:37:08, joth wrote: > uber-nit: this is not strictly always going to result in deadlock, it's a > straight hang (waiting for an event that is not going to happen). Comment removed when creating a separate private method for all the return cases. https://codereview.chromium.org/11823027/diff/47002/ipc/ipc_channel_proxy.cc#... ipc/ipc_channel_proxy.cc:264: if (!handled && message.is_sync()) { On 2013/01/18 01:31:24, Leandro Graciá Gil wrote: > Should this also happen in other places returning in this method? (e.g. if there > is no listener) Done. https://codereview.chromium.org/11823027/diff/47002/ipc/ipc_channel_proxy.cc#... ipc/ipc_channel_proxy.cc:271: } On 2013/01/18 01:37:08, joth wrote: > this seems pretty reasonable to me, FWIW, but I'd recommend landing it first in > its own patch? splitting the android-only parts from this common code would help > keep the two apart as they cycle through the bots. We can if either you or John have a strong opinion on this, although I included this in the same patch in order to make sure the trybots run the same WebView tests that exposed the problem.
On 2013/01/18 18:49:32, Leandro Graciá Gil wrote: > On 2013/01/18 18:37:29, John Abd-El-Malek wrote: > > On 2013/01/18 00:31:54, Leandro Graciá Gil wrote: > > > jam: just before landing this patch we found some corner cases in WebView > > tests > > > where legacy APIs triggering synchronous browser -> renderer IPCs were sent > > > before the RenderView has been created. This caused problems like crashes > and > > > deadlocks. Looking further for the cause I found 2 IPC issues: > > > > > > 1. When a synchronous message is not handled (e.g. no RenderView yet) we > don't > > > ever send a reply back, causing a deadlock. > > > > > > 2. There are some cases in RenderProcessHostImpl where we enqueue the > messages > > > if we're not ready to send them to try again later. That's ok for > asynchronous > > > messages, but synchronous ones should simply fail and return false. > > > > > > Could you please take a look to the changes in render_process_host_impl.cc > and > > > ipc_channel_proxy.cc? Thanks. > > > > i'm wary of adding complexity to shared code to support the legacy webview > case. > > > > can you fix this by other ways, i.e. where you send the sync IPCs for the > legacy > > IPCs, check if a RVH is live or not? > > We can try to workaround this, but that won't solve the real problem and might > lead to further and unexpected deadlocks in the future. We know there are 2 real > issues in sync IPCs and only one of them should be limited to the Browser -> > Renderer case (which happens to be WebView-only for now, but it's not > necessarily "for WebView"). We can't guarantee either us or somebody else will > bump into them in the future in some other way. > > I'm open to any suggestions you might have to reduce complexity, but I do think > we should fix the real problem rather than trying to work around it. Also I'm happy to move this into a separate non-WebView patch. It was included here just for the purpose of having the trybots check the tests that originally exposed the problem. As I said this should not be a WebView-only issue, it just was the first to expose it.
John - I'm I right in thinking your wariness directed mostly to RPH change, not the ipc/proxy change? To me, the improvement in https://codereview.chromium.org/11823027/diff/69017/ipc/ipc_channel_proxy.ccs... sound, and worthwhile on its own merits. (?) We can work around the other part in render_process_host_impl.cc as you suggest, in order to unblock this patch. Let us know your thoughts. Thanks! On 18 January 2013 10:54, <leandrogracia@chromium.org> wrote: > On 2013/01/18 18:49:32, Leandro Graciá Gil wrote: > >> On 2013/01/18 18:37:29, John Abd-El-Malek wrote: >> > On 2013/01/18 00:31:54, Leandro Graciá Gil wrote: >> > > jam: just before landing this patch we found some corner cases in >> WebView >> > tests >> > > where legacy APIs triggering synchronous browser -> renderer IPCs were >> > sent > >> > > before the RenderView has been created. This caused problems like >> crashes >> and >> > > deadlocks. Looking further for the cause I found 2 IPC issues: >> > > >> > > 1. When a synchronous message is not handled (e.g. no RenderView yet) >> we >> don't >> > > ever send a reply back, causing a deadlock. >> > > >> > > 2. There are some cases in RenderProcessHostImpl where we enqueue the >> messages >> > > if we're not ready to send them to try again later. That's ok for >> asynchronous >> > > messages, but synchronous ones should simply fail and return false. >> > > >> > > Could you please take a look to the changes in >> render_process_host_impl.cc >> and >> > > ipc_channel_proxy.cc? Thanks. >> > >> > i'm wary of adding complexity to shared code to support the legacy >> webview >> case. >> > >> > can you fix this by other ways, i.e. where you send the sync IPCs for >> the >> legacy >> > IPCs, check if a RVH is live or not? >> > > We can try to workaround this, but that won't solve the real problem and >> might >> lead to further and unexpected deadlocks in the future. We know there are >> 2 >> > real > >> issues in sync IPCs and only one of them should be limited to the Browser >> -> >> Renderer case (which happens to be WebView-only for now, but it's not >> necessarily "for WebView"). We can't guarantee either us or somebody else >> will >> bump into them in the future in some other way. >> > > I'm open to any suggestions you might have to reduce complexity, but I do >> > think > >> we should fix the real problem rather than trying to work around it. >> > > Also I'm happy to move this into a separate non-WebView patch. It was > included > here just for the purpose of having the trybots check the tests that > originally > exposed the problem. As I said this should not be a WebView-only issue, it > just > was the first to expose it. > > https://codereview.chromium.**org/11823027/<https://codereview.chromium.org/1... >
On 2013/01/18 21:20:45, joth wrote: > John - I'm I right in thinking your wariness directed mostly to RPH change, > not the ipc/proxy change? > > To me, the improvement in > https://codereview.chromium.org/11823027/diff/69017/ipc/ipc_channel_proxy.ccs... > sound, and worthwhile on its own merits. (?) > > We can work around the other part in render_process_host_impl.cc as you > suggest, in order to unblock this patch. > > Let us know your thoughts. > > Thanks! I feel that the IPCChannelProxy change will mask the higher-level error, which is where it should be handled. put another way, would you have caught this error if the send failed instead of hung?
On 2013/01/18 21:31:18, John Abd-El-Malek wrote: > On 2013/01/18 21:20:45, joth wrote: > > John - I'm I right in thinking your wariness directed mostly to RPH change, > > not the ipc/proxy change? > > > > To me, the improvement in > > > https://codereview.chromium.org/11823027/diff/69017/ipc/ipc_channel_proxy.ccs... > > sound, and worthwhile on its own merits. (?) > > > > We can work around the other part in render_process_host_impl.cc as you > > suggest, in order to unblock this patch. > > > > Let us know your thoughts. > > > > Thanks! > > I feel that the IPCChannelProxy change will mask the higher-level error, which > is where it should be handled. > > put another way, would you have caught this error if the send failed instead of > hung? Actually, failing instead of hanging would have been the desired behaviour.
On 2013/01/18 21:35:27, Leandro Graciá Gil wrote: > Actually, failing instead of hanging would have been the desired behaviour. Let me provide some more context on the scenario where this is happening. This synchronous IPC can be triggered as part of calling capturePicture() from the Android Framework, which provides an Android Picture that is able to replay any current view contents. However, it's completely valid for this to happen in offscreen WebViews that have not loaded any url. Consequently, these WebViews might not have a RenderView created and this is correct. If there isn't any we don't want to hung, just to fail and provide no picture as a result as there is none. So, as you can see there are scenarios where it is actually valid to trigger synchronous IPCs without anybody to listen them in the other side. We can fail (and we can handle that if we need to), but we can't ever hung.
joth: could you re-review the AwContents native and Java classes? I've refactored them and added support for SW rendering when no platform methods are available so that upstream tests can run fine.
https://codereview.chromium.org/11823027/diff/81001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/11823027/diff/81001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:1019: sBitmapCache = new SoftReference<Bitmap>(bitmap); looks like I read these patches out of order... sBitmapCache is complex as it's used by two different flows now, one of which (capturePicture fallback) holds potentially long-lived references to the bitmap. we shouldn't really cache the bitmap in that case. maybe we don't care as it's just a fallback mode? But maybe all round easiest just to get rid of this cache https://codereview.chromium.org/11823027/diff/81001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:1037: private static Picture recordRasterizedBitmap(Bitmap bitmap) { recordBitmapIntoPicture ? https://codereview.chromium.org/11823027/diff/81001/android_webview/native/aw... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/11823027/diff/81001/android_webview/native/aw... android_webview/native/aw_contents.cc:530: LOG(WARNING) << "Skia native versions are not compatible."; LOG_IF(!g_is_skia_version_compatible, WARNING) << ... https://codereview.chromium.org/11823027/diff/81001/android_webview/native/aw... android_webview/native/aw_contents.cc:980: // TODO(leandrogracia): uncomment when sw rendering uses Ubercompositor. I'd rather we leave "TODO: uncomment..." for things we know will land imminently (i.e. patch already in the works) otherwise the code can go stale. For this, just say: TODO when SW rendering uses the compositor rather than picture rasterization, send update the renderer side with the correct picture listener state. (For now, we always leave render picture listener enabled).
https://codereview.chromium.org/11823027/diff/81001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/11823027/diff/81001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:1019: sBitmapCache = new SoftReference<Bitmap>(bitmap); On 2013/01/22 00:05:41, joth wrote: > looks like I read these patches out of order... > sBitmapCache is complex as it's used by two different flows now, one of which > (capturePicture fallback) holds potentially long-lived references to the bitmap. > we shouldn't really cache the bitmap in that case. maybe we don't care as it's > just a fallback mode? But maybe all round easiest just to get rid of this cache I think you're right. This caching can cause problems like, for example, when we're provided a Picture-backed Canvas in onDraw and we re-use the bitmap later. Probably will be best if we remove the soft reference. https://codereview.chromium.org/11823027/diff/81001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:1037: private static Picture recordRasterizedBitmap(Bitmap bitmap) { On 2013/01/22 00:05:41, joth wrote: > recordBitmapIntoPicture ? Yes, much better. Done. https://codereview.chromium.org/11823027/diff/81001/android_webview/native/aw... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/11823027/diff/81001/android_webview/native/aw... android_webview/native/aw_contents.cc:530: LOG(WARNING) << "Skia native versions are not compatible."; On 2013/01/22 00:05:41, joth wrote: > LOG_IF(!g_is_skia_version_compatible, WARNING) << ... Didn't know that one. Done. (Btw, it's LOG_IF(severity, condition)). https://codereview.chromium.org/11823027/diff/81001/android_webview/native/aw... android_webview/native/aw_contents.cc:980: // TODO(leandrogracia): uncomment when sw rendering uses Ubercompositor. On 2013/01/22 00:05:41, joth wrote: > I'd rather we leave "TODO: uncomment..." for things we know will land imminently > (i.e. patch already in the works) otherwise the code can go stale. For this, > just say: > TODO when SW rendering uses the compositor rather than picture rasterization, > send update the renderer side with the correct picture listener state. (For now, > we always leave render picture listener enabled). Done.
On 2013/01/19 01:11:22, Leandro Graciá Gil wrote: > On 2013/01/18 21:35:27, Leandro Graciá Gil wrote: > > Actually, failing instead of hanging would have been the desired behaviour. > > Let me provide some more context on the scenario where this is happening. This > synchronous IPC can be triggered as part of calling capturePicture() from the > Android Framework, which provides an Android Picture that is able to replay any > current view contents. > > However, it's completely valid for this to happen in offscreen WebViews that > have not loaded any url. Consequently, these WebViews might not have a > RenderView created and this is correct. If there isn't any we don't want to > hung, just to fail and provide no picture as a result as there is none. > > So, as you can see there are scenarios where it is actually valid to trigger > synchronous IPCs without anybody to listen them in the other side. We can fail > (and we can handle that if we need to), but we can't ever hung. it seems that a webview which hasn't loaded any url can check that a RenderView exists or not, and if not, not send the ipc?
On 2013/01/22 07:01:01, John Abd-El-Malek wrote: > On 2013/01/19 01:11:22, Leandro Graciá Gil wrote: > > On 2013/01/18 21:35:27, Leandro Graciá Gil wrote: > > > Actually, failing instead of hanging would have been the desired behaviour. > > > > Let me provide some more context on the scenario where this is happening. This > > synchronous IPC can be triggered as part of calling capturePicture() from the > > Android Framework, which provides an Android Picture that is able to replay > any > > current view contents. > > > > However, it's completely valid for this to happen in offscreen WebViews that > > have not loaded any url. Consequently, these WebViews might not have a > > RenderView created and this is correct. If there isn't any we don't want to > > hung, just to fail and provide no picture as a result as there is none. > > > > So, as you can see there are scenarios where it is actually valid to trigger > > synchronous IPCs without anybody to listen them in the other side. We can fail > > (and we can handle that if we need to), but we can't ever hung. > > it seems that a webview which hasn't loaded any url can check that a RenderView > exists or not, and if not, not send the ipc? We could workaround it, yes. But we're leaving the hole open to fall into it again in the future, either ourselves, someone else working in WebView in the future or anyone else using synchronous IPC. Also, a deadlock in WebView would not be limited to the view itself, but to the entire app using it and possibly to the entire Android View System if this happens during a GL functor call. Are you sure this is the best way to solve the issue?
On 2013/01/18 21:31:18, John Abd-El-Malek wrote: > On 2013/01/18 21:20:45, joth wrote: > > John - I'm I right in thinking your wariness directed mostly to RPH change, > > not the ipc/proxy change? > > > > To me, the improvement in > > > https://codereview.chromium.org/11823027/diff/69017/ipc/ipc_channel_proxy.ccs... > > sound, and worthwhile on its own merits. (?) > > > > We can work around the other part in render_process_host_impl.cc as you > > suggest, in order to unblock this patch. > > > > Let us know your thoughts. > > > > Thanks! > > I feel that the IPCChannelProxy change will mask the higher-level error, which > is where it should be handled. > > put another way, would you have caught this error if the send failed instead of > hung? Would it be better if we DCHECKed in ChannelProxy::Context::SendFailedSyncMessageReply? That way we'd still catch the issue in debug mode but would not hang in release mode.
On 2013/01/22 17:16:21, Martin Kosiba wrote: > On 2013/01/18 21:31:18, John Abd-El-Malek wrote: > > On 2013/01/18 21:20:45, joth wrote: > > > John - I'm I right in thinking your wariness directed mostly to RPH change, > > > not the ipc/proxy change? > > > > > > To me, the improvement in > > > > > > https://codereview.chromium.org/11823027/diff/69017/ipc/ipc_channel_proxy.ccs... > > > sound, and worthwhile on its own merits. (?) > > > > > > We can work around the other part in render_process_host_impl.cc as you > > > suggest, in order to unblock this patch. > > > > > > Let us know your thoughts. > > > > > > Thanks! > > > > I feel that the IPCChannelProxy change will mask the higher-level error, which > > is where it should be handled. > > > > put another way, would you have caught this error if the send failed instead > of > > hung? > > Would it be better if we DCHECKed in > ChannelProxy::Context::SendFailedSyncMessageReply? That way we'd still catch the > issue in debug mode but would not hang in release mode. i checked to other owners of ipc. they felt the same way (that doing this at the ipc layer would mask errors at a higher level). dcheck sounds fine to me though
On 2013/01/23 01:05:36, John Abd-El-Malek wrote: > On 2013/01/22 17:16:21, Martin Kosiba wrote: > > On 2013/01/18 21:31:18, John Abd-El-Malek wrote: > > > On 2013/01/18 21:20:45, joth wrote: > > > > John - I'm I right in thinking your wariness directed mostly to RPH > change, > > > > not the ipc/proxy change? > > > > > > > > To me, the improvement in > > > > > > > > > > https://codereview.chromium.org/11823027/diff/69017/ipc/ipc_channel_proxy.ccs... > > > > sound, and worthwhile on its own merits. (?) > > > > > > > > We can work around the other part in render_process_host_impl.cc as you > > > > suggest, in order to unblock this patch. > > > > > > > > Let us know your thoughts. > > > > > > > > Thanks! > > > > > > I feel that the IPCChannelProxy change will mask the higher-level error, > which > > > is where it should be handled. > > > > > > put another way, would you have caught this error if the send failed instead > > of > > > hung? > > > > Would it be better if we DCHECKed in > > ChannelProxy::Context::SendFailedSyncMessageReply? That way we'd still catch > the > > issue in debug mode but would not hang in release mode. > > i checked to other owners of ipc. they felt the same way (that doing this at the > ipc layer would mask errors at a higher level). dcheck sounds fine to me though Ok, I'll go that way. What should be the correct way to check from the browser if there is a RenderView listening on the other side? We also need to avoid the cases where the messages are enqueued during initialization.
On 2013/01/23 02:00:31, Leandro Graciá Gil wrote: > Ok, I'll go that way. What should be the correct way to check from the browser > if there is a RenderView listening on the other side? We also need to avoid the > cases where the messages are enqueued during initialization. I'm not sure how you guys have this implemented where there's no RenderView on the other side, so hard to say. perhaps look at IsRenderViewLive?
On 2013/01/24 01:47:04, John Abd-El-Malek wrote: > On 2013/01/23 02:00:31, Leandro Graciá Gil wrote: > > Ok, I'll go that way. What should be the correct way to check from the browser > > if there is a RenderView listening on the other side? We also need to avoid > the > > cases where the messages are enqueued during initialization. > > > I'm not sure how you guys have this implemented where there's no RenderView on > the other side, so hard to say. perhaps look at IsRenderViewLive? jam: I've updated the patch to not send synchronous IPCs when the RenderView is not ready or there is not a connection. This completely prevents the crashes and hungs in the tests. However, in order to catch any similar problems in the future I've added some NOTREACHED to the proposed solution. This way we will quickly catch any issues but not hung in release mode. Let me know if you're ok with this. I have tested the test to pass without changes to the IPC files and they do, so the workaround is working.
On 2013/01/29 17:19:31, Leandro Graciá Gil wrote: > On 2013/01/24 01:47:04, John Abd-El-Malek wrote: > > On 2013/01/23 02:00:31, Leandro Graciá Gil wrote: > > > Ok, I'll go that way. What should be the correct way to check from the > browser > > > if there is a RenderView listening on the other side? We also need to avoid > > the > > > cases where the messages are enqueued during initialization. > > > > > > I'm not sure how you guys have this implemented where there's no RenderView on > > the other side, so hard to say. perhaps look at IsRenderViewLive? > > jam: I've updated the patch to not send synchronous IPCs when the RenderView is > not ready or there is not a connection. This completely prevents the crashes and > hungs in the tests. However, in order to catch any similar problems in the > future I've added some NOTREACHED to the proposed solution. This way we will > quickly catch any issues but not hung in release mode. Let me know if you're ok > with this. I have tested the test to pass without changes to the IPC files and > they do, so the workaround is working. Looks like IPCSyncChannelTest.NoHang is reaching the no-listener case flakily. jam: do you know if this is expected or perhaps is a bug in the test itself?
On 2013/01/29 18:05:13, Leandro Graciá Gil wrote: > On 2013/01/29 17:19:31, Leandro Graciá Gil wrote: > > On 2013/01/24 01:47:04, John Abd-El-Malek wrote: > > > On 2013/01/23 02:00:31, Leandro Graciá Gil wrote: > > > > Ok, I'll go that way. What should be the correct way to check from the > > browser > > > > if there is a RenderView listening on the other side? We also need to > avoid > > > the > > > > cases where the messages are enqueued during initialization. > > > > > > > > > I'm not sure how you guys have this implemented where there's no RenderView > on > > > the other side, so hard to say. perhaps look at IsRenderViewLive? > > > > jam: I've updated the patch to not send synchronous IPCs when the RenderView > is > > not ready or there is not a connection. This completely prevents the crashes > and > > hungs in the tests. However, in order to catch any similar problems in the > > future I've added some NOTREACHED to the proposed solution. This way we will > > quickly catch any issues but not hung in release mode. Let me know if you're > ok > > with this. I have tested the test to pass without changes to the IPC files and > > they do, so the workaround is working. > > Looks like IPCSyncChannelTest.NoHang is reaching the no-listener case flakily. > jam: do you know if this is expected or perhaps is a bug in the test itself? I'm limiting the reply safeguard to the non-handled message case, the one that originally exposed the problem. Taking a look to the sync channel unit tests, it seems the listener can be null on closed channels that have pending messages to be processed, and that should not trigger a NOTREACHED. jam: if instead you prefer me to remove any of the debug checks let me know and I'll leave the original IPC code untouched.
Apparently, many different unit tests are now hitting the DCHECK of synchronous message not managed. I wonder why we never found an issue before and maybe it's worth investigating further, but for now I'll be removing any IPC DCHECKs and limiting this patch again to android_webview code. jam: sorry for all this going back and forth and thanks for your help on the issue. If there are no further concerns and the bots are happy, I'll land this today.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/11823027/98004
Message was sent while issue was closed.
Change committed as 179693 |
