Fix Native Messaging to handle missing caller RenderWidgetHost.

Fix Native Messaging to handle missing caller RenderWidgetHost.

content::RenderWidgetHost::FromID() may return NULL and
MessageService::OpenChannelToNativeApp() wasn't handling that case
properly.

I couldn't reproduce the problem, but I think this change should
fix it.

BUG=420495
Committed: https://crrev.com/6147e9407e49f2d27d618dddc9d156bbe2a17d28
Cr-Commit-Position: refs/heads/master@{#307964}

[Sergey Ulanov, 2014-12-11 17:14:32]

sergeyu@chromium.org changed reviewers:
+ kalman@chromium.org

[not at google - send to devlin, 2014-12-11 17:24:43]

https://codereview.chromium.org/791343003/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/messaging/message_service.cc (right):

https://codereview.chromium.org/791343003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/messaging/message_service.cc:410: : NULL;
Are we supposed to use nullptr these days? I'm losing track.

Anyway, I may be missing something here, but I traced through the call stack and
I can't see the native view actually used anywhere.

Under the assumption I *am* missing something, then this change lgtm, though it
seems weird passing a potentially NULL native view. You could document in
NativeMessageHost::Create that the view can be null. Or even better... delete it
entirely :)

[Sergey Ulanov, 2014-12-11 19:25:29]

https://codereview.chromium.org/791343003/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/messaging/message_service.cc (right):

https://codereview.chromium.org/791343003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/messaging/message_service.cc:410: : NULL;
On 2014/12/11 17:24:42, kalman wrote:
> Are we supposed to use nullptr these days? I'm losing track.

Yes, it should be nullptr. This is my first change in more than 2 month
(parental leave), so I forgot about it. Thanks for catching it.

> 
> Anyway, I may be missing something here, but I traced through the call stack
and
> I can't see the native view actually used anywhere.

It's used only on windows to pass chrome window handle to the native process,
e.g. so that it can set correct parent for UAC prompts. See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

> 
> Under the assumption I *am* missing something, then this change lgtm, though
it
> seems weird passing a potentially NULL native view. You could document in
> NativeMessageHost::Create that the view can be null. Or even better... delete
it
> entirely :)

Added a comment in native_process_launcer.h

[Sergey Ulanov, 2014-12-11 19:26:15]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-12-11 19:27:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/791343003/20001

[not at google - send to devlin, 2014-12-11 20:19:46]

https://codereview.chromium.org/791343003/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/messaging/message_service.cc (right):

https://codereview.chromium.org/791343003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/messaging/message_service.cc:410: : NULL;

> 
> Added a comment in native_process_launcer.h

Where is native_view used then?

[commit-bot: I haz the power, 2014-12-11 20:37:27]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-12-11 20:38:26]

Patchset 2 (id:??) landed as
https://crrev.com/6147e9407e49f2d27d618dddc9d156bbe2a17d28
Cr-Commit-Position: refs/heads/master@{#307964}

[Sergey Ulanov, 2014-12-11 20:44:30]

On 2014/12/11 20:19:46, kalman wrote:
>
https://codereview.chromium.org/791343003/diff/1/chrome/browser/extensions/ap...
> File chrome/browser/extensions/api/messaging/message_service.cc (right):
> 
>
https://codereview.chromium.org/791343003/diff/1/chrome/browser/extensions/ap...
> chrome/browser/extensions/api/messaging/message_service.cc:410: : NULL;
> 
> > 
> > Added a comment in native_process_launcer.h
> 
> Where is native_view used then?

It's converted to window_handle here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
and then passed to the called process here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

[not at google - send to devlin, 2014-12-11 20:59:01]

On 2014/12/11 20:44:30, Sergey Ulanov wrote:
> On 2014/12/11 20:19:46, kalman wrote:
> >
>
https://codereview.chromium.org/791343003/diff/1/chrome/browser/extensions/ap...
> > File chrome/browser/extensions/api/messaging/message_service.cc (right):
> > 
> >
>
https://codereview.chromium.org/791343003/diff/1/chrome/browser/extensions/ap...
> > chrome/browser/extensions/api/messaging/message_service.cc:410: : NULL;
> > 
> > > 
> > > Added a comment in native_process_launcer.h
> > 
> > Where is native_view used then?
> 
> It's converted to window_handle here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> and then passed to the called process here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

Oh right. Weird, code search didn't pick that up.

[Sergey Ulanov, 2014-12-11 22:35:42]

On Thu, Dec 11, 2014 at 12:59 PM, <kalman@chromium.org> wrote:

> Oh right. Weird, code search didn't pick that up.
>

That's because code search only sees code that's compiled on Linux.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.