aw: Fix multiprocess flag usage

aw: Fix multiprocess flag usage

Stop using kSingleProcess, which does not mean what we think it means.
Pass the correct webview process type to breakpad, so that microdumps
contain the correct information.

BUG=708773
Review-Url: https://codereview.chromium.org/2806733002
Cr-Commit-Position: refs/heads/master@{#463302}
Committed: https://chromium.googlesource.com/chromium/src/+/90bce3cf40ac6f6cfaf1e3b742bf47f86f2d4e6e

[Tobias Sargeant, 2017-04-07 16:55:49]

The CQ bit was checked by tobiasjs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 16:56:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 17:21:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 17:21:19]

Dry run: This issue passed the CQ dry run.

[Tobias Sargeant, 2017-04-10 10:59:27]

Description was changed from

==========
aw: Fix multiprocess flag usage

Stop using kSingleProcess, which does not mean what we think it means.

BUG=708773
==========

to

==========
aw: Fix multiprocess flag usage

Stop using kSingleProcess, which does not mean what we think it means.
Pass the correct webview process type to breakpad, so that microdumps
contain the correct information.

BUG=708773
==========

[Tobias Sargeant, 2017-04-10 10:59:27]

tobiasjs@chromium.org changed reviewers:
+ rsesek@chromium.org, torne@chromium.org

[Tobias Sargeant, 2017-04-10 11:01:55]

PTAL. At present all crashes are listed as browser crashes regardless of whether
they're single process or multi process because we're not handling the command
line flag correctly, and we're not passing the right string to breakpad.

[Torne, 2017-04-10 14:06:30]

One overall question: I can't actually find where we fiddle with kSingleProcess
in the first place..

https://codereview.chromium.org/2806733002/diff/20001/android_webview/browser...
File android_webview/browser/aw_content_browser_client.cc (left):

https://codereview.chromium.org/2806733002/diff/20001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:274: NOTREACHED() <<
"Android WebView does not support multi-process yet";
This is pretty weird; I assume we never reached this line? The message seems
misleading. I'm okay with removing it since it doesn't seem to be accomplishing
anything, but if you figured out why this was here in the first place I'm
curious :)

https://codereview.chromium.org/2806733002/diff/20001/components/crash/conten...
File components/crash/content/app/breakpad_linux.cc (right):

https://codereview.chromium.org/2806733002/diff/20001/components/crash/conten...
components/crash/content/app/breakpad_linux.cc:1957: process_type ==
kBrowserProcessType ||
How annoying would it be to change all the uses of empty string to "browser" so
this isn't OS-specific? (maybe not in this change since I assume we're going to
pick this to 58) Worth it?

[Tobias Sargeant, 2017-04-10 14:11:29]

On 2017/04/10 14:06:30, Torne wrote:
> One overall question: I can't actually find where we fiddle with
kSingleProcess
> in the first place..

Yes.. Exactly..

[Tobias Sargeant, 2017-04-10 14:21:15]

https://codereview.chromium.org/2806733002/diff/20001/android_webview/browser...
File android_webview/browser/aw_content_browser_client.cc (left):

https://codereview.chromium.org/2806733002/diff/20001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:274: NOTREACHED() <<
"Android WebView does not support multi-process yet";
On 2017/04/10 14:06:29, Torne wrote:
> This is pretty weird; I assume we never reached this line? The message seems
> misleading. I'm okay with removing it since it doesn't seem to be
accomplishing
> anything, but if you figured out why this was here in the first place I'm
> curious :)

Since kSingleProcess was never set, I presume we never reached this.

The message itself goes way back (2012) and was unconditional. Mikhail added the
condition. I don't quite understand the point, because it seems like this method
is only be called when we're going to start a child process.

https://codereview.chromium.org/2806733002/diff/20001/components/crash/conten...
File components/crash/content/app/breakpad_linux.cc (right):

https://codereview.chromium.org/2806733002/diff/20001/components/crash/conten...
components/crash/content/app/breakpad_linux.cc:1957: process_type ==
kBrowserProcessType ||
On 2017/04/10 14:06:29, Torne wrote:
> How annoying would it be to change all the uses of empty string to "browser"
so
> this isn't OS-specific? (maybe not in this change since I assume we're going
to
> pick this to 58) Worth it?

It seems like it would be worth it. It's not our code though. I'm pretty sure
this CL removes the last webview use of "" as a process type.

I'll take a look with lower priority.

[Tobias Sargeant, 2017-04-10 14:21:29]

tobiasjs@chromium.org changed reviewers:
+ boliu@chromium.org

[Tobias Sargeant, 2017-04-10 14:21:51]

Adding Bo because he might know more of the history.

[Tobias Sargeant, 2017-04-10 14:21:51]

Adding Bo because he might know more of the history.

[Torne, 2017-04-10 14:25:14]

OK; this CL LGTM, but Robert may have more to say about breakpad process types
:)

[Robert Sesek, 2017-04-10 15:17:18]

LGTM

https://codereview.chromium.org/2806733002/diff/20001/components/crash/conten...
File components/crash/content/app/breakpad_linux.cc (right):

https://codereview.chromium.org/2806733002/diff/20001/components/crash/conten...
components/crash/content/app/breakpad_linux.cc:1957: process_type ==
kBrowserProcessType ||
On 2017/04/10 14:06:29, Torne wrote:
> How annoying would it be to change all the uses of empty string to "browser"
so
> this isn't OS-specific? (maybe not in this change since I assume we're going
to
> pick this to 58) Worth it?

The process_type.empty() meaning browser is a baked-in assumption of Chrome,
since it stems from the fact that the browser process has no --type= switch.
Whether or not that's worth changing at the crash reporting level is debatable.

[boliu, 2017-04-10 16:20:35]

Summary on chat is that the command line switch changes appear to be no-op
afaict. But it's fine to land this, and torne already approved

kSingleProcess is equivalent to !kWebViewSandboxedRenderer afaict.
kSingleProcess is set in [1] and the bool used to add it is read at [2]

[1]
https://cs.chromium.org/chromium/src/content/browser/android/content_startup_...

[2]
https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/an...

[Tobias Sargeant, 2017-04-10 16:23:39]

The CQ bit was checked by tobiasjs@chromium.org

[commit-bot: I haz the power, 2017-04-10 16:24:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 17:15:48]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491841419808500,
"parent_rev": "a6a6e611adeab2c8dd64a4229743c61a3c1c7a22", "commit_rev":
"90bce3cf40ac6f6cfaf1e3b742bf47f86f2d4e6e"}

[commit-bot: I haz the power, 2017-04-10 17:16:38]

Description was changed from

==========
aw: Fix multiprocess flag usage

Stop using kSingleProcess, which does not mean what we think it means.
Pass the correct webview process type to breakpad, so that microdumps
contain the correct information.

BUG=708773
==========

to

==========
aw: Fix multiprocess flag usage

Stop using kSingleProcess, which does not mean what we think it means.
Pass the correct webview process type to breakpad, so that microdumps
contain the correct information.

BUG=708773

Review-Url: https://codereview.chromium.org/2806733002
Cr-Commit-Position: refs/heads/master@{#463302}
Committed:
https://chromium.googlesource.com/chromium/src/+/90bce3cf40ac6f6cfaf1e3b742bf...
==========

[commit-bot: I haz the power, 2017-04-10 17:16:39]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/90bce3cf40ac6f6cfaf1e3b742bf...