Fix a null AppCacheBackendImpl ptr crash in CompleteCrossSiteTransfer and re…

Fix a null AppCacheBackendImpl ptr crash in AppCacheRequestHandler::CompleteCrossSiteTransfer.

The crash was caused by a storage partition mismatch. Now instead of the browser crashing, we report instances of the mismatch using bad_message mechanism which terminates the offending child process.

BUG=568439
TBR=isherman

Committed: https://crrev.com/054c2fd45d455f7df0ed30a19c32a97f681be6fc
Cr-Commit-Position: refs/heads/master@{#369927}

[michaeln, 2016-01-07 00:08:27]

michaeln@chromium.org changed reviewers:
+ jsbell@chromium.org, nasko@chromium.org

[michaeln, 2016-01-07 00:08:27]

ptal

I'm not sure the circumstance in which this happens but apparently it does when
multiple extensions are involved and crossSite complications are in play.

@nasko can you take a look too since it  involves cross site navigation stuff?

[nasko, 2016-01-07 00:27:02]

On 2016/01/07 00:08:27, michaeln wrote:
> ptal
> 
> I'm not sure the circumstance in which this happens but apparently it does
when
> multiple extensions are involved and crossSite complications are in play.
> 
> @nasko can you take a look too since it  involves cross site navigation stuff?

This will fix a crash, right? It looks like returning early is already a
supported pattern for this code, so it looks fine to me. Do you know what the
early return will manifest as? Hanging request?

[michaeln, 2016-01-07 01:47:00]

On 2016/01/07 00:27:02, nasko wrote:
> On 2016/01/07 00:08:27, michaeln wrote:
> > ptal
> > 
> > I'm not sure the circumstance in which this happens but apparently it does
> when
> > multiple extensions are involved and crossSite complications are in play.
> > 
> > @nasko can you take a look too since it  involves cross site navigation
stuff?
> 
> This will fix a crash, right? It looks like returning early is already a
> supported pattern for this code, so it looks fine to me. Do you know what the
> early return will manifest as? Hanging request?

I don't understand how that ptr can be null, so no, i don't know what the
downstream consequences of the early return are. I haven't repro'd the crash.

Seemingly, we're processing an ipc message from a particular renderer whose ipc
channel doesn't have an appcache message filter (AppcacheDispatcherHost)
attached to it. Makes no sense, but we're doing this in the context of
extension'ware so idk?

I'm pretty sure the exensions involved are not using appcache. I'd expect
MaybeInterceptXxx methods to return NULL. Maybe we should write code to
explicitly make that happen this case?

[michaeln, 2016-01-07 23:51:38]

On 2016/01/07 01:47:00, michaeln wrote:
> On 2016/01/07 00:27:02, nasko wrote:
> > On 2016/01/07 00:08:27, michaeln wrote:
> > > ptal
> > > 
> > > I'm not sure the circumstance in which this happens but apparently it does
> > when
> > > multiple extensions are involved and crossSite complications are in play.
> > > 
> > > @nasko can you take a look too since it  involves cross site navigation
> stuff?
> > 
> > This will fix a crash, right? It looks like returning early is already a
> > supported pattern for this code, so it looks fine to me. Do you know what
the
> > early return will manifest as? Hanging request?
> 
> I don't understand how that ptr can be null, so no, i don't know what the
> downstream consequences of the early return are. I haven't repro'd the crash.
> 
> Seemingly, we're processing an ipc message from a particular renderer whose
ipc
> channel doesn't have an appcache message filter (AppcacheDispatcherHost)
> attached to it. Makes no sense, but we're doing this in the context of
> extension'ware so idk?
> 
> I'm pretty sure the exensions involved are not using appcache. I'd expect
> MaybeInterceptXxx methods to return NULL. Maybe we should write code to
> explicitly make that happen this case?

Wait a minute... the chromecast extension does use appcache so bailing out early
may avoid a crash, but it isn't going to help correctness?

[michaeln, 2016-01-14 22:42:32]

ptal, using bad_message as mentioned in crbug

[michaeln, 2016-01-14 22:46:42]

Description was changed from

==========
Test for a null AppCacheBackendImpl ptr in
AppCacheRequestHandler::CompleteCrossSiteTransfer and return early in that case.

BUG=568439
==========

to

==========
Fix a null AppCacheBackendImpl ptr crash in
AppCacheRequestHandler::CompleteCrossSiteTransfer.

The crash was caused by a storage partition mismatch. Now instead of the browser
crashing, we report instances of the mismatch using bad_message mechanism which
terminates the offending child process.

BUG=568439
==========

[jsbell, 2016-01-14 22:52:29]

lgtm

https://codereview.chromium.org/1566713004/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_request_handler.h (right):

https://codereview.chromium.org/1566713004/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_request_handler.h:61: bool
SanityCheckIsSameService(AppCacheService* service) {
Add brief documentation here, i.e. why anyone would call this?

[michaeln, 2016-01-15 00:03:34]

The CQ bit was checked by michaeln@chromium.org

[michaeln, 2016-01-15 00:03:34]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/1566713004/#ps60001
(title: "doc comment")

[commit-bot: I haz the power, 2016-01-15 00:06:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1566713004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1566713004/60001

[commit-bot: I haz the power, 2016-01-15 00:30:48]

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

[commit-bot: I haz the power, 2016-01-15 00:30:49]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[michaeln, 2016-01-15 01:38:02]

ooops, @nasko need a content owner approval too

[nasko, 2016-01-15 19:38:24]

LGTM once a typo is fixed.

https://codereview.chromium.org/1566713004/diff/60001/content/browser/appcach...
File content/browser/appcache/appcache_interceptor.cc (right):

https://codereview.chromium.org/1566713004/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_interceptor.cc:89:
bad_message::ACI_WRONG_STORAGE_PARITION);
You have a missing T in PARTITION.

[michaeln, 2016-01-15 21:31:23]

https://codereview.chromium.org/1566713004/diff/60001/content/browser/bad_mes...
File content/browser/bad_message.h (right):

https://codereview.chromium.org/1566713004/diff/60001/content/browser/bad_mes...
content/browser/bad_message.h:133: // reason. After making changes, you MUST
update histograms.xml by running:
thnx, i'll update histograms.xml too

[michaeln, 2016-01-15 22:12:48]

Description was changed from

==========
Fix a null AppCacheBackendImpl ptr crash in
AppCacheRequestHandler::CompleteCrossSiteTransfer.

The crash was caused by a storage partition mismatch. Now instead of the browser
crashing, we report instances of the mismatch using bad_message mechanism which
terminates the offending child process.

BUG=568439
==========

to

==========
Fix a null AppCacheBackendImpl ptr crash in
AppCacheRequestHandler::CompleteCrossSiteTransfer.

The crash was caused by a storage partition mismatch. Now instead of the browser
crashing, we report instances of the mismatch using bad_message mechanism which
terminates the offending child process.

BUG=568439
TBR=isherman
==========

[michaeln, 2016-01-15 22:15:17]

> thnx, i'll update histograms.xml too

TBR'ing isherman for the histograms change

[michaeln, 2016-01-15 22:15:34]

The CQ bit was checked by michaeln@chromium.org

[michaeln, 2016-01-15 22:15:34]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1566713004/#ps80001
(title: " ")

[commit-bot: I haz the power, 2016-01-15 22:16:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1566713004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1566713004/80001

[commit-bot: I haz the power, 2016-01-15 23:53:56]

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

[commit-bot: I haz the power, 2016-01-15 23:53:59]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[michaeln, 2016-01-16 00:08:31]

The CQ bit was checked by michaeln@chromium.org

[michaeln, 2016-01-16 00:08:31]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1566713004/#ps100001
(title: " ")

[commit-bot: I haz the power, 2016-01-16 00:09:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1566713004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1566713004/100001

[commit-bot: I haz the power, 2016-01-16 00:11:22]

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

[commit-bot: I haz the power, 2016-01-16 00:11:22]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[michaeln, 2016-01-16 01:13:07]

The CQ bit was checked by michaeln@chromium.org

[michaeln, 2016-01-16 01:13:07]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1566713004/#ps120001
(title: " ")

[commit-bot: I haz the power, 2016-01-16 01:14:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1566713004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1566713004/120001

[commit-bot: I haz the power, 2016-01-16 02:33:58]

Description was changed from

==========
Fix a null AppCacheBackendImpl ptr crash in
AppCacheRequestHandler::CompleteCrossSiteTransfer.

The crash was caused by a storage partition mismatch. Now instead of the browser
crashing, we report instances of the mismatch using bad_message mechanism which
terminates the offending child process.

BUG=568439
TBR=isherman
==========

to

==========
Fix a null AppCacheBackendImpl ptr crash in
AppCacheRequestHandler::CompleteCrossSiteTransfer.

The crash was caused by a storage partition mismatch. Now instead of the browser
crashing, we report instances of the mismatch using bad_message mechanism which
terminates the offending child process.

BUG=568439
TBR=isherman
==========

[commit-bot: I haz the power, 2016-01-16 02:33:59]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-01-16 02:34:57]

Description was changed from

==========
Fix a null AppCacheBackendImpl ptr crash in
AppCacheRequestHandler::CompleteCrossSiteTransfer.

The crash was caused by a storage partition mismatch. Now instead of the browser
crashing, we report instances of the mismatch using bad_message mechanism which
terminates the offending child process.

BUG=568439
TBR=isherman
==========

to

==========
Fix a null AppCacheBackendImpl ptr crash in
AppCacheRequestHandler::CompleteCrossSiteTransfer.

The crash was caused by a storage partition mismatch. Now instead of the browser
crashing, we report instances of the mismatch using bad_message mechanism which
terminates the offending child process.

BUG=568439
TBR=isherman

Committed: https://crrev.com/054c2fd45d455f7df0ed30a19c32a97f681be6fc
Cr-Commit-Position: refs/heads/master@{#369927}
==========

[commit-bot: I haz the power, 2016-01-16 02:34:58]

Patchset 7 (id:??) landed as
https://crrev.com/054c2fd45d455f7df0ed30a19c32a97f681be6fc
Cr-Commit-Position: refs/heads/master@{#369927}