Expose details for appcache error events [Chromium]

Expose details for appcache error events

Chromium side. Would land after Blink side.

BUG=342555
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260732

[jsbell, 2014-02-21 23:48:23]

https://codereview.chromium.org/164933002/diff/170001/webkit/browser/appcache...
File webkit/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/164933002/diff/170001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:438: // TODO: Should both of
these errors be generated?
The code here will result in two errors - CancelAllMasterEntryFetches() and
NotifyAllError() both call SendErrorNotifications().

As you say, I may just be clueless about where the errors get routed to.

[michaeln, 2014-02-22 00:19:39]

i haven't looked closely yet, just responding to the TODO

https://codereview.chromium.org/164933002/diff/170001/webkit/browser/appcache...
File webkit/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/164933002/diff/170001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:438: // TODO: Should both of
these errors be generated?
On 2014/02/21 23:48:23, jsbell wrote:
> The code here will result in two errors - CancelAllMasterEntryFetches() and
> NotifyAllError() both call SendErrorNotifications().
> 
> As you say, I may just be clueless about where the errors get routed to.

Like everything... not as simple as it should be. There are multiple recipients
of the set of progress/error/yada events generated by an update job. Not all
recipients are equal, some see a different event stream than others (you have to
read the spec real close, but there it is).

What CancelAllMasterEntryFetches does is raise the error event into all pages
that caused a new master entry to be added, and then unplugs those recipients
from receiving any other events. The way in which "recipients are not equal" has
to do with whether a new master entry is being established vs a preexisting
cached page was loaded.

NotifyAllError then spams the remaining recipients with the same error.

In this particular HandleCacheFailure case, see same error is being given to all
recipeints, but there are other cases where the page with the new master will
see an error, but other pages may continue and ultimately see success. (see
HandleMasterEntryFetchCompleted)

Each recipient should see only one terminating event.

Also, a potential source of "update hangs" is bugs in this notifier plumbing
that just drops the ball and leaves some should be recipients out.

[jsbell, 2014-02-22 00:23:09]

On 2014/02/22 00:19:39, michaeln wrote:
> i haven't looked closely yet, just responding to the TODO
> 
>
https://codereview.chromium.org/164933002/diff/170001/webkit/browser/appcache...
> File webkit/browser/appcache/appcache_update_job.cc (right):
> 
>
https://codereview.chromium.org/164933002/diff/170001/webkit/browser/appcache...
> webkit/browser/appcache/appcache_update_job.cc:438: // TODO: Should both of
> these errors be generated?
> On 2014/02/21 23:48:23, jsbell wrote:
> > The code here will result in two errors - CancelAllMasterEntryFetches() and
> > NotifyAllError() both call SendErrorNotifications().
> > 
> > As you say, I may just be clueless about where the errors get routed to.
> 
> Like everything... not as simple as it should be. There are multiple
recipients
> of the set of progress/error/yada events generated by an update job. Not all
> recipients are equal, some see a different event stream than others (you have
to
> read the spec real close, but there it is).

Thanks. This raises the question of what details should be given to recipients -
does it make sense for them all to get the same details, or should they have
different "views"? (e.g. one sees a specific resource fail, the other sees a
generic cache fail)

[michaeln, 2014-02-22 00:52:12]

> Thanks. This raises the question of what details should be given to recipients
-
> does it make sense for them all to get the same details, or should they have
> different "views"? (e.g. one sees a specific resource fail, the other sees a
> generic cache fail)

Idk, your CL is stuffing the error pipe at the source of the problem with actual
details and we can follow what goes where and see if it makes sense. I'll look
at the places where different recipients get treated differently more closely.

[michaeln, 2014-02-22 01:18:49]

I got distracted with the "Hmmm" observation.

https://codereview.chromium.org/164933002/diff/170001/webkit/browser/appcache...
File webkit/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/164933002/diff/170001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:737: // TODO: Is this a duplicate
error?
No duplicates, the recipients of the error sent by
host_notifier.SendErrorNotifications on line 725 have been unplugged so to speak
(see line 716), so the error sent via HandleCacheFailure will not be delivered
to them.

https://codereview.chromium.org/164933002/diff/170001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:864: inprogress_cache_ =
newest_cache;
Hmmm... unrelated to this CL, but you know the DiscardInProgressCache crasher...
i think i see how that happens now.

StoreGroupAndCache() puts the object in the state the Discard method is not
prepared for. The shutdown logic involves killing pending tasks without running
their completion functions. This completion function restores the object to the
state that DiscardInProgressCache is coded for, but it's skipped... and
DiscardInProgressCache is confused.

[michaeln, 2014-02-27 23:16:05]

https://codereview.chromium.org/164933002/diff/170001/webkit/browser/appcache...
File webkit/browser/appcache/appcache_host.cc (right):

https://codereview.chromium.org/164933002/diff/170001/webkit/browser/appcache...
webkit/browser/appcache/appcache_host.cc:121: "Policy",
ah... an outlier error type, good find!

https://codereview.chromium.org/164933002/diff/290001/webkit/browser/appcache...
File webkit/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/164933002/diff/290001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:438: // TODO: Should both of
these errors be generated?
This should be ok, CancelAllMasterEntryFetches is delivered to one set of
clients and NotifyAllError is sent to another. No client sees two errors, and
the same error is seen by both sets.

https://codereview.chromium.org/164933002/diff/290001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:523: ERROR_MANIFEST_FETCH,
s/b ERROR_MANIFEST_OBSOLETE?

This is another of those differences in behavior
- new master entry pages see and error event
- existing cached pages see an obsolete event

https://codereview.chromium.org/164933002/diff/290001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:525: GURL(),
Maybe we shouldn't populate the url attribute for all reasons? If the reason is
particular to the manifesturl thats should be a known enough value to the
caller.

manifest_url_ should be available here, but the result code is gone at this
point

see const GURL manifest_url_;  // here for easier access

https://codereview.chromium.org/164933002/diff/290001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:636: HandleCacheFailure(message,
There are a handful of reasons a fetcher could have failed, fetcher->result()
can contain these kinds of errors

REDIRECT_ERROR (got a 30x which is verbotten, response_code = ?)
SERVER_ERROR (got non-200 response, response_code > 0)
NETWORK_ERROR (got no response whatsoever, response_code = -1)
DISKCACHE_ERROR (failed to write what we got to disk, response_code = ?)

What do we want to say about 'reason' to in the resulting error event?

ERROR_RESOURCE_FETCH definitely describes the operation or step that went wrong,
but thats a little different than the reason for failure. The consumer might be
able to discern the 'reason' based on the value in response_code.

I'm not sure what we want to do here, just bringing it up. If 'message' is also
exposed finer grained distinctions could be made there.

https://codereview.chromium.org/164933002/diff/290001/webkit/common/appcache/...
File webkit/common/appcache/appcache_interfaces.h (right):

https://codereview.chromium.org/164933002/diff/290001/webkit/common/appcache/...
webkit/common/appcache/appcache_interfaces.h:76: ERROR_LAST = ERROR_INTERNAL
I'm looking at this set of Reasons and comparing to the set of ResultTypes we
log in UMA. I think your high level reasons look pretty good, but the lower
level distinctions would be nice to convey since a fetch operation can fail for
a variety of reasons.

 enum ErrorReason {
  68   ERROR_MANIFEST_FETCH,
  69   ERROR_MANIFEST_INVALID,
  70   ERROR_MANIFEST_CHANGED,
  71   ERROR_MANIFEST_OBSOLETE,
  72   ERROR_RESOURCE_FETCH,
  73   ERROR_QUOTA_EXCEEDED,
  74   ERROR_POLICY,
  75   ERROR_INTERNAL,  // this pretty much means failed to read/write disk

  enum ResultType {
    UPDATE_OK,
    DB_ERROR,
    DISKCACHE_ERROR,
    QUOTA_ERROR,
    REDIRECT_ERROR,
    MANIFEST_ERROR,
    NETWORK_ERROR,
    SERVER_ERROR,
  };

[jsbell, 2014-02-28 22:45:59]

https://codereview.chromium.org/164933002/diff/290001/webkit/browser/appcache...
File webkit/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/164933002/diff/290001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:523: ERROR_MANIFEST_FETCH,
On 2014/02/27 23:16:06, michaeln wrote:
> s/b ERROR_MANIFEST_OBSOLETE?

Whoops, yes.

> This is another of those differences in behavior
> - new master entry pages see and error event
> - existing cached pages see an obsolete event

Ah, thanks.

https://codereview.chromium.org/164933002/diff/290001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:525: GURL(),
On 2014/02/27 23:16:06, michaeln wrote:
> Maybe we shouldn't populate the url attribute for all reasons? If the reason
is
> particular to the manifesturl thats should be a known enough value to the
> caller.

Agreed. Given that the message has "404 or 410" I was wondering if we could pipe
it through, but since we can't then we don't need to send the URL along either.

https://codereview.chromium.org/164933002/diff/290001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:636: HandleCacheFailure(message,
On 2014/02/27 23:16:06, michaeln wrote:
> There are a handful of reasons a fetcher could have failed, fetcher->result()
> can contain these kinds of errors
> 
> REDIRECT_ERROR (got a 30x which is verbotten, response_code = ?)
> SERVER_ERROR (got non-200 response, response_code > 0)

^^^ combined with response_code, I think a single type for those is sufficient.
(i.e. if it's a 5xx they know the server is busted, 4xx they know the config is
wrong, 3xx they should read the spec.)

Then again: if redirect is explicitly called out in the spec, having a distinct
error type for it makes sense. Thoughts?

> NETWORK_ERROR (got no response whatsoever, response_code = -1)

^^^ Makes sense to distinguish in some way. Or we could say ERROR_RESOURCE_FETCH
and the response_code is 0 ? i.e. this is the same basic category of error.

> DISKCACHE_ERROR (failed to write what we got to disk, response_code = ?)
> 

And this seems like it's the ERROR_INTERNAL bucket, i.e. you as a developer
can't fix this. 


> What do we want to say about 'reason' to in the resulting error event?
> 
> ERROR_RESOURCE_FETCH definitely describes the operation or step that went
wrong,
> but thats a little different than the reason for failure. The consumer might
be
> able to discern the 'reason' based on the value in response_code.

That's what I was leaning towards, apart from the cache failure.

> I'm not sure what we want to do here, just bringing it up. If 'message' is
also
> exposed finer grained distinctions could be made there.
>

https://codereview.chromium.org/164933002/diff/290001/webkit/common/appcache/...
File webkit/common/appcache/appcache_interfaces.h (right):

https://codereview.chromium.org/164933002/diff/290001/webkit/common/appcache/...
webkit/common/appcache/appcache_interfaces.h:76: ERROR_LAST = ERROR_INTERNAL
Agreed, see discussion under ERROR_RESOURCE_FETCH

[michaeln, 2014-03-01 00:20:01]

https://codereview.chromium.org/164933002/diff/310001/webkit/browser/appcache...
File webkit/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/164933002/diff/310001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:138: request->Cancel();
It might be a matter of not calling cancel here, so the status code shows thru
down under OnResponseCompleted in the Job method. OnResponseCompleted deletes
the request which will cancel things as well.

https://codereview.chromium.org/164933002/diff/310001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:560: message,
ERROR_MANIFEST_INVALID, manifest_url_, 0, MANIFEST_ERROR);
maybe use an empty GURL() here too?

https://codereview.chromium.org/164933002/diff/310001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:637: result == DISKCACHE_ERROR ?
ERROR_INTERNAL : ERROR_RESOURCE_FETCH,
This looks reasonable to me, there is something that needs to be checked. In the
REDIRECT_ERROR case, i'm not sure what value will be reported for response_code?
We'll need to be sure that the 30x shows up in that case. Also i'm not sure what
value will show up in response_code for for DISKCACHE_ERROR, less important what
gets put in response_code in that case i suppose?

That's what the ?'s meant here...

REDIRECT_ERROR (got a 30x which is verbotten, response_code = ?)
SERVER_ERROR (got non-200 response, response_code > 0)
NETWORK_ERROR (got no response whatsoever, response_code = -1)
DISKCACHE_ERROR (failed to write what we got to disk, response_code = ?)

https://codereview.chromium.org/164933002/diff/310001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:677: int response_code =
request->status().is_success()
in cases when the fetcher cancels the request, i think we'll be reporting -1 for
status code. upon seeing a redirect, i think the fetcher cancels

[jsbell, 2014-03-01 00:53:06]

https://codereview.chromium.org/164933002/diff/310001/webkit/browser/appcache...
File webkit/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/164933002/diff/310001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:560: message,
ERROR_MANIFEST_INVALID, manifest_url_, 0, MANIFEST_ERROR);
On 2014/03/01 00:20:02, michaeln wrote:
> maybe use an empty GURL() here too?

Will do.

https://codereview.chromium.org/164933002/diff/310001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:637: result == DISKCACHE_ERROR ?
ERROR_INTERNAL : ERROR_RESOURCE_FETCH,
On 2014/03/01 00:20:02, michaeln wrote:
> Also i'm not sure what
> value will show up in response_code for for DISKCACHE_ERROR, less important
what
> gets put in response_code in that case i suppose?

D'oh, right. I'll fix.

BTW, I'm making the minimal/obvious changes, but I'm not dismissing the overall
discussion.

https://codereview.chromium.org/164933002/diff/310001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:677: int response_code =
request->status().is_success()
On 2014/03/01 00:20:02, michaeln wrote:
> in cases when the fetcher cancels the request, i think we'll be reporting -1
for
> status code. upon seeing a redirect, i think the fetcher cancels

Okay, I'll try and trace this through. We'll need tests, obviously.

[michaeln, 2014-03-01 01:34:55]

https://codereview.chromium.org/164933002/diff/310001/webkit/browser/appcache...
File webkit/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/164933002/diff/310001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:677: int response_code =
request->status().is_success()
There are a lot of updatejob unittests. Checking of expected values could
probably be added to those tests.

Whoa... I think I might have an unexpected explanation for some of the 'hung'
updates. Look at CacheAttemptFetchManifestFailTest and look at the set of
expected events... only CHECKING. This tests has that expectation, got me
wondering if this (screwed up) behavior might actually be the specified
behavior?

[jsbell, 2014-03-03 23:10:18]

https://codereview.chromium.org/164933002/diff/310001/webkit/browser/appcache...
File webkit/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/164933002/diff/310001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:677: int response_code =
request->status().is_success()
Nice find. Hrm.... spec's table (6.7.1.2 Event summary) says that 'checking'
will be followed by one of 'noupdate', 'downloading', 'obsolete' or 'error', but
the spec may be inconsistent. ManifestRedirectTest also looks like it's only
expecting 'checking' ?

Though it looks like the event is cancelable, although I don't see the test
triggering that in any way.

Spec certainly seems to indicate that 404/410 should fire 'obsolete',  other 4xx
or 5xx or parse failure 'error', 304 (or byte-wise identical) as 'noupdate',
otherwise 'downloading'

[jsbell, 2014-03-03 23:23:57]

Back to enumerating possible errors, it looks like these are the sources of
error per spec:

* fetching the manifest fails due to 404 or 410 or equivalent:
* upgrade attempt, and resource download failed
* otherwise, and resource download failed
* fetching manifest fails due to non-404/410 (i.e. 4xx, 5xx, DNS, timeout, user
cancel, parse failure, etc)
* explicit abort() call from script
* resource fetch fails (4xx, 5xx, DNS, timeout, user cancel, etc.) or redirect
and resource is explicit entry or fallback entry
* user agent failure (e.g. quota)
* manifest changed on re-fetch

Aligning an enum with those would probably make the change easiest to spec.

[michaeln, 2014-03-04 02:45:55]

On 2014/03/03 23:23:57, jsbell wrote:
> Back to enumerating possible errors, it looks like these are the sources of
> error per spec:
> 
> * fetching the manifest fails due to 404 or 410 or equivalent:
> * upgrade attempt, and resource download failed
> * otherwise, and resource download failed

is the second one--> a cache attempt and resource download failed

> * fetching manifest fails due to non-404/410 (i.e. 4xx, 5xx, DNS, timeout,
user
> cancel, parse failure, etc)
> * explicit abort() call from script

this one is not implemented in chrome

> * resource fetch fails (4xx, 5xx, DNS, timeout, user cancel, etc.) or redirect
> and resource is explicit entry or fallback entry

how is this different than the earlier "and resource download failed"s ?

> * user agent failure (e.g. quota)
> * manifest changed on re-fetch
> 
> Aligning an enum with those would probably make the change easiest to spec.

There is also a botched manifest file (wrong signature). Do you wan to
add/remove anything from this set of reasons or spell out how these map to a
"std" set reasons?

  67 enum ErrorReason {
  68   ERROR_MANIFEST_FETCH,
  69   ERROR_MANIFEST_INVALID,
  70   ERROR_MANIFEST_CHANGED,
  71   ERROR_MANIFEST_OBSOLETE,
  72   ERROR_RESOURCE_FETCH,
  73   ERROR_QUOTA_EXCEEDED,
  74   ERROR_POLICY,
  75   ERROR_INTERNAL,
  76   ERROR_LAST = ERROR_INTERNAL
  77 };

[michaeln, 2014-03-04 02:57:30]

>> Whoa... I think I might have an unexpected explanation for some
>
> Spec certainly seems to indicate that 404/410 should fire 'obsolete',  other
4xx
> or 5xx or parse failure 'error', 304 (or byte-wise identical) as 'noupdate',
> otherwise 'downloading'

Sorry for the distraction. These are unittests and not system tests, the lack of
an ERROR_EVENT expectation is due to how those particular tests are composed.
Events are generally delivered to "associated" hosts, but in those tests there
is no cache association.

[jsbell, 2014-03-12 19:04:00]

So... you can specify an off-domain resource in MANIFEST. Do we need to filter
out details from those for script, as that would be a cross-domain information
leak. I.e. you could use it to probe whether a user gets a 200 or a 403 for a
given URL?

It seems you can probe success or failure of a URL access via img already...

[jsbell, 2014-03-12 21:40:42]

> > * otherwise, and resource download failed
> 
> is the second one--> a cache attempt and resource download failed


> > * resource fetch fails (4xx, 5xx, DNS, timeout, user cancel, etc.) or
redirect
> > and resource is explicit entry or fallback entry
> 
> how is this different than the earlier "and resource download failed"s ?
> 

I put together
https://docs.google.com/a/chromium.org/document/d/1nlk7WgRD3d0ZcfK1xrwBFVZ3DI...
to explain WTF I was trying to accomplish here.

> There is also a botched manifest file (wrong signature). Do you wan to
> add/remove anything from this set of reasons or spell out how these map to a
> "std" set reasons?

Yes. :) I think we need to start from the spec and work backwards to the code,
or we'll get standards push-back.

[michaeln, 2014-03-12 22:06:32]

On 2014/03/12 19:04:00, jsbell wrote:
> So... you can specify an off-domain resource in MANIFEST. Do we need to filter
> out details from those for script, as that would be a cross-domain information
> leak. I.e. you could use it to probe whether a user gets a 200 or a 403 for a
> given URL?

That's a really good question. I don't think obscuring the URL accomplishes
anything (the malicious party could still probe one cross-origin resource per
manifest), but we could hide the status code.

> It seems you can probe success or failure of a URL access via img already...

[jsbell, 2014-03-14 22:25:51]

FYI, I just updated the patches (Chromium and Blink) to align with the spec
proposal.

https://docs.google.com/document/d/1nlk7WgRD3d0ZcfK1xrwBFVZ3DI_e44j7QoMd5gAJC...

[michaeln, 2014-03-15 00:19:09]

this lgtm, modulo the cross domian question and making sure 3xx's are reported

[jsbell, 2014-03-17 18:25:53]

On 2014/03/15 00:19:09, michaeln wrote:
> this lgtm, modulo the cross domian question and making sure 3xx's are reported

FYI, I confirmed that 3xx's are NOT plumbed through right now. Working on it.

(Adding more tests on the blink side, too)

[jsbell, 2014-03-24 20:57:33]

New patch. This one plumbs through an "is_cross_origin" bit and lets the
renderer blank out the status and message given to script - full details are
still logged, though.

Since this was too many fields to send through IPC separately I added
appcache::ErrorDetails struct and used it everywhere to simplify method
signatures throughout.

[michaeln, 2014-03-27 01:05:36]

lgtm before and still does now

https://codereview.chromium.org/164933002/diff/430001/content/child/appcache/...
File content/child/appcache/web_application_cache_host_impl.cc (right):

https://codereview.chromium.org/164933002/diff/430001/content/child/appcache/...
content/child/appcache/web_application_cache_host_impl.cc:151:
static_cast<ErrorReason>(details.reason), details.url, 0, WebString());
lgtm

https://codereview.chromium.org/164933002/diff/430001/webkit/browser/appcache...
File webkit/browser/appcache/appcache_unittest.cc (right):

https://codereview.chromium.org/164933002/diff/430001/webkit/browser/appcache...
webkit/browser/appcache/appcache_unittest.cc:27: const appcache::ErrorDetails&
details)
don't think appcache:: is needed (not sure why its in OnCacheSelected)

https://codereview.chromium.org/164933002/diff/430001/webkit/browser/appcache...
File webkit/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/164933002/diff/430001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:117: canceled_response_code_(-1)
{}
redirect_response_code_ might be a better name?

https://codereview.chromium.org/164933002/diff/430001/webkit/common/appcache/...
File webkit/common/appcache/appcache_interfaces.cc (right):

https://codereview.chromium.org/164933002/diff/430001/webkit/common/appcache/...
webkit/common/appcache/appcache_interfaces.cc:63: bool in_is_cross_origin)
nit: indent looks funky, maybe back the arglist up to 4 spaces in from method
sig?

[jsbell, 2014-03-27 19:23:23]

https://codereview.chromium.org/164933002/diff/430001/webkit/browser/appcache...
File webkit/browser/appcache/appcache_unittest.cc (right):

https://codereview.chromium.org/164933002/diff/430001/webkit/browser/appcache...
webkit/browser/appcache/appcache_unittest.cc:27: const appcache::ErrorDetails&
details)
On 2014/03/27 01:05:37, michaeln wrote:
> don't think appcache:: is needed (not sure why its in OnCacheSelected)

Done.

https://codereview.chromium.org/164933002/diff/430001/webkit/browser/appcache...
File webkit/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/164933002/diff/430001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:117: canceled_response_code_(-1)
{}
On 2014/03/27 01:05:37, michaeln wrote:
> redirect_response_code_ might be a better name?

Done.

https://codereview.chromium.org/164933002/diff/430001/webkit/common/appcache/...
File webkit/common/appcache/appcache_interfaces.cc (right):

https://codereview.chromium.org/164933002/diff/430001/webkit/common/appcache/...
webkit/common/appcache/appcache_interfaces.cc:63: bool in_is_cross_origin)
On 2014/03/27 01:05:37, michaeln wrote:
> nit: indent looks funky, maybe back the arglist up to 4 spaces in from method
> sig?

Done. I blame clang format.

[jsbell, 2014-03-27 19:30:42]

I probably need to restore passing through the resource URL, to be consistent.

https://codereview.chromium.org/164933002/diff/450001/webkit/browser/appcache...
File webkit/browser/appcache/appcache_update_job.cc (left):

https://codereview.chromium.org/164933002/diff/450001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:434: const GURL&
failed_resource_url) {
Since I was already passing the failed resource URL through in ErrorDetails I
eliminated this parameter. However...

https://codereview.chromium.org/164933002/diff/450001/webkit/browser/appcache...
File webkit/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/164933002/diff/450001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:510: manifest_url_,
Here the manifest URL is passed through instead of empty-URL

https://codereview.chromium.org/164933002/diff/450001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:647: message, UNKNOWN_ERROR,
GURL(), 0, is_cross_origin),
Here an empty URL is passed through instead of the resource url

https://codereview.chromium.org/164933002/diff/450001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:766: request->url(),
Here the request URL is passed through instead of blank...

[michaeln, 2014-03-27 19:38:50]

On 2014/03/27 19:30:42, jsbell wrote:
> I probably need to restore passing through the resource URL, to be consistent.

Oh, i hadn't noticed those changes. So long as the url property on the JSEvent
object is properly populated (or not) depending on the error type, the value
present in the Details object is a seperate matter.

[jsbell, 2014-03-27 20:29:27]

On 2014/03/27 19:38:50, michaeln wrote:
> On 2014/03/27 19:30:42, jsbell wrote:
> > I probably need to restore passing through the resource URL, to be
consistent.

Restored it in the latest patch.

> Oh, i hadn't noticed those changes. So long as the url property on the JSEvent
> object is properly populated (or not) depending on the error type, the value
> present in the Details object is a seperate matter.

The URL is sometimes populated in ErrorDetails when it wasn't passed through as
a failed_resource_url and vice versa. The differences were important, e.g. when
the failure was internal we still want to UMA-stat the % complete, but we don't
want to pass the URL to script.

[michaeln, 2014-03-27 21:19:16]

On 2014/03/27 20:29:27, jsbell wrote:
> On 2014/03/27 19:38:50, michaeln wrote:
> > On 2014/03/27 19:30:42, jsbell wrote:
> > > I probably need to restore passing through the resource URL, to be
> consistent.
> 
> Restored it in the latest patch.
> 
> > Oh, i hadn't noticed those changes. So long as the url property on the
JSEvent
> > object is properly populated (or not) depending on the error type, the value
> > present in the Details object is a seperate matter.
> 
> The URL is sometimes populated in ErrorDetails when it wasn't passed through
as
> a failed_resource_url and vice versa. The differences were important, e.g.
when
> the failure was internal we still want to UMA-stat the % complete, but we
don't
> want to pass the URL to script.

I see, different uses for that value and it's absence means different things.
Restoring it as a separate param makes sense.

[jsbell, 2014-03-28 16:01:29]

tsepez@ - Can you pleas review the IPC changes in
content/common/appcache_messages.h ?

[Tom Sepez, 2014-03-28 17:27:33]

On 2014/03/28 16:01:29, jsbell wrote:
> tsepez@ - Can you pleas review the IPC changes in
> content/common/appcache_messages.h ?
I'm concerned about your is_cross_origin flag.  Am I correct in assuming that
the other fields are still populated even when this is set and it is up to the
renderer to ignore them?  If so, I'd be happier if they were blanked on the
browser side, with the bit set instead (e.g. was_cross_origin so you got
nothing).

[jsbell, 2014-03-28 17:35:22]

On 2014/03/28 17:27:33, Tom Sepez wrote:
> On 2014/03/28 16:01:29, jsbell wrote:
> > tsepez@ - Can you pleas review the IPC changes in
> > content/common/appcache_messages.h ?
> I'm concerned about your is_cross_origin flag.  Am I correct in assuming that
> the other fields are still populated even when this is set and it is up to the
> renderer to ignore them?  If so, I'd be happier if they were blanked on the
> browser side, with the bit set instead (e.g. was_cross_origin so you got
> nothing).

That's correct, and intentional. See:

https://codereview.chromium.org/164933002/diff/470001/content/child/appcache/...

One of the fields (`message`) is already (i.e. prior to this patch) plumbed
through to the renderer. It is shown on the JS console but is not exposed to
script. It's a free-form string and generally includes the other new field that
isn't exposed to script (an `status` code). So we could blank out `status` on
the browser side, but we'd likely still be sending the same data in the string,
so it didn't seem worthwhile.

Thoughts?

[jsbell, 2014-03-28 17:38:55]

On 2014/03/28 17:35:22, jsbell wrote:
> That's correct, and intentional. See:
> 
>
https://codereview.chromium.org/164933002/diff/470001/content/child/appcache/...
> 
> One of the fields (`message`) is already (i.e. prior to this patch) plumbed
> through to the renderer. It is shown on the JS console but is not exposed to
> script. It's a free-form string and generally includes the other new field
that
> isn't exposed to script (an `status` code). So we could blank out `status` on
> the browser side, but we'd likely still be sending the same data in the
string,
> so it didn't seem worthwhile.

Whoops, to clarify:

The `message` field is currently plumbed through the renderer but - prior to
this patch - not exposed to script. With this patch, the `message` field *is*
exposed to script, but only if `is_cross_domain` is false. So in either case
we're still sending it to the renderer - so we can't blank it out - and we need
the extra `is_cross_domain` to decide whether to expose it to script.

We could still blank out `status` on the browser side, but again that data is
usually present in the `message` which is already sent through.

[Tom Sepez, 2014-03-28 17:43:16]

OK, let's go with this the way it is for now. Messages LGTM.
We may need to rethink this in a site-isolation world.

[jsbell, 2014-03-31 16:54:51]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-03-31 16:55:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/164933002/470001

[jsbell, 2014-03-31 20:24:32]

The CQ bit was unchecked by jsbell@chromium.org

[jsbell, 2014-03-31 22:02:19]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-03-31 22:03:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/164933002/490001

[commit-bot: I haz the power, 2014-04-01 01:35:47]

Change committed as 260732