Service worker registration error support

Service worker registration error support

This patch introduces use of WebServiceWorkerError as a means to propagate an
error from the browser to the renderer, and further reflect this error via
WebCallbacks into Blink

BUG=285976
R=jam@chromium.org, michaeln@chromium.org, tsepez@chromium.org
TBR=abarth

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

[alecflett, 2013-10-16 00:24:27]

Here's the chrome side of the error handler, with a working unittest. (The
unittest itself may seem a little trivial, but it was killing me to be checking
in code without tests!)

[alecflett, 2013-10-16 00:26:03]

Also, I'm not particularly psyched to be touching DEPS files here, but it seemed
better than introducing boilerplate error enums to match webkit (which would
have turned an explicit dependency into an implicit one, which isn't better IMO)
- at least there is precident and our file is POD and enums, like the DEPS
comment says.

[alecflett, 2013-10-16 01:09:09]

Ok, new patch is up to reflect ownership change from
https://codereview.chromium.org/27278002/

[alecflett, 2013-10-25 18:33:49]

tsepez@chromium.org: this is a followon to http://crrev.com/25008006 to
propagate the errors back to the caller

[Tom Sepez, 2013-10-25 18:46:58]

LGTM.  Please forgive the nits.

https://codereview.chromium.org/26442004/diff/159001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/26442004/diff/159001/content/browser/DEPS#new...
content/browser/DEPS:42: "+third_party/WebKit/public/platform/WebScreenInfo.h",
nit: alphabetize.

https://codereview.chromium.org/26442004/diff/159001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/26442004/diff/159001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:68:
ASCIIToUTF16("ServiceWorker is disabled")));
nit: maybe a kSomething constant instead of the string literal, as its used
twice. Or not.

https://codereview.chromium.org/26442004/diff/159001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.h (right):

https://codereview.chromium.org/26442004/diff/159001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.h:55: void
OnRegistered(int32 thread_id, int32 request_id, int64 service_worker_id);
nit: I liked the old names better.  Up to you.

https://codereview.chromium.org/26442004/diff/159001/content/common/service_w...
File content/common/service_worker_messages.h (right):

https://codereview.chromium.org/26442004/diff/159001/content/common/service_w...
content/common/service_worker_messages.h:11: #include
"third_party/WebKit/public/platform/WebServiceWorkerError.h"
nit: include order.

https://codereview.chromium.org/26442004/diff/159001/content/common/service_w...
content/common/service_worker_messages.h:46: // Any kind of registration error
from RegisterServiceWorker /
nit: this isn't a sentence, its a sentence fragment.  Do you mean "Sent when
there is any kind of ..."

https://codereview.chromium.org/26442004/diff/159001/content/common/service_w...
content/common/service_worker_messages.h:48: // actual reason for the error, or
even be split if there is a
nit: do you mean, "In the future, this could expand ... "?

https://codereview.chromium.org/26442004/diff/159001/content/common/service_w...
content/common/service_worker_messages.h:55: string16 /* message */)
nit: do we need to pass a string here? Does it contain information beyond what
is represented by the ErrorType?  If not, maybe a mapping from code to string on
the other side of the IPC is desirable.

https://codereview.chromium.org/26442004/diff/159001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/26442004/diff/159001/content/content_tests.gy...
content/content_tests.gypi:458:
'browser/service_worker/service_worker_dispatcher_host_unittest.cc',
nit: alphabetize.

[alecflett, 2013-10-25 20:05:02]

thanks!

https://codereview.chromium.org/26442004/diff/159001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/26442004/diff/159001/content/browser/DEPS#new...
content/browser/DEPS:42: "+third_party/WebKit/public/platform/WebScreenInfo.h",
On 2013/10/25 18:46:59, Tom Sepez wrote:
> nit: alphabetize.

Done.

https://codereview.chromium.org/26442004/diff/159001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/26442004/diff/159001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:68:
ASCIIToUTF16("ServiceWorker is disabled")));
On 2013/10/25 18:46:59, Tom Sepez wrote:
> nit: maybe a kSomething constant instead of the string literal, as its used
> twice. Or not.

This merely a temporary state until the flag is removed... leaving it up to the
compiler to de-dupe the literal until that happens

https://codereview.chromium.org/26442004/diff/159001/content/common/service_w...
File content/common/service_worker_messages.h (right):

https://codereview.chromium.org/26442004/diff/159001/content/common/service_w...
content/common/service_worker_messages.h:46: // Any kind of registration error
from RegisterServiceWorker /
On 2013/10/25 18:46:59, Tom Sepez wrote:
> nit: this isn't a sentence, its a sentence fragment.  Do you mean "Sent when
> there is any kind of ..."

Done.

https://codereview.chromium.org/26442004/diff/159001/content/common/service_w...
content/common/service_worker_messages.h:48: // actual reason for the error, or
even be split if there is a
On 2013/10/25 18:46:59, Tom Sepez wrote:
> nit: do you mean, "In the future, this could expand ... "?

Done.

https://codereview.chromium.org/26442004/diff/159001/content/common/service_w...
content/common/service_worker_messages.h:55: string16 /* message */)
On 2013/10/25 18:46:59, Tom Sepez wrote:
> nit: do we need to pass a string here? Does it contain information beyond what
> is represented by the ErrorType?  If not, maybe a mapping from code to string
on
> the other side of the IPC is desirable.

Yes, there is such a mapping in Blink.

[michaeln, 2013-10-25 21:41:24]

https://codereview.chromium.org/26442004/diff/159001/content/common/service_w...
File content/common/service_worker_messages.h (right):

https://codereview.chromium.org/26442004/diff/159001/content/common/service_w...
content/common/service_worker_messages.h:55: string16 /* message */)
On 2013/10/25 20:05:02, alecflett wrote:
> On 2013/10/25 18:46:59, Tom Sepez wrote:
> > nit: do we need to pass a string here? Does it contain information beyond
what
> > is represented by the ErrorType?  If not, maybe a mapping from code to
string
> on
> > the other side of the IPC is desirable.

There is a generic mapping from Type to string, but the message adds more
detailed information (eventually for things like particular url values).

https://codereview.chromium.org/26442004/diff/319001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/26442004/diff/319001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:60: if
(scope.GetOrigin() != script_url.GetOrigin())
this can use the error return path now and probably should come after the
enabled check

https://codereview.chromium.org/26442004/diff/319001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/26442004/diff/319001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:22:
ASSERT_TRUE(dir_.CreateUniqueTempDir());
we can probably pass an empty path for these tests and avoid disk access

https://codereview.chromium.org/26442004/diff/319001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:23:
context_ = new ServiceWorkerContext(dir_.path(), nullptr);
NULL in chromium speak

https://codereview.chromium.org/26442004/diff/319001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/26442004/diff/319001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:140: if (!callbacks)
do we need the test for !callback?

https://codereview.chromium.org/26442004/diff/319001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.h (right):

https://codereview.chromium.org/26442004/diff/319001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.h:61: string16 message);
s/b const string16& here

https://codereview.chromium.org/26442004/diff/319001/content/common/service_w...
File content/common/service_worker_messages.h (right):

https://codereview.chromium.org/26442004/diff/319001/content/common/service_w...
content/common/service_worker_messages.h:50: // between registration and
unregistration errors.
The first sentence looks good enough.

https://codereview.chromium.org/26442004/diff/319001/content/common/service_w...
content/common/service_worker_messages.h:55: string16 /* message */)
should we use utf8 here and upsample at the WebKit API boundary?

[michaeln, 2013-10-25 22:02:18]

lgtm (modulo nits)

[alecflett, 2013-10-25 23:41:55]

https://codereview.chromium.org/26442004/diff/319001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/26442004/diff/319001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:22:
ASSERT_TRUE(dir_.CreateUniqueTempDir());
On 2013/10/25 21:41:25, michaeln wrote:
> we can probably pass an empty path for these tests and avoid disk access

Done.

https://codereview.chromium.org/26442004/diff/319001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:23:
context_ = new ServiceWorkerContext(dir_.path(), nullptr);
On 2013/10/25 21:41:25, michaeln wrote:
> NULL in chromium speak

Done.

https://codereview.chromium.org/26442004/diff/319001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/26442004/diff/319001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:140: if (!callbacks)
On 2013/10/25 21:41:25, michaeln wrote:
> do we need the test for !callback?

We've needed to do this for security reasons in IDB - error checking on the IPC
boundary.

https://codereview.chromium.org/26442004/diff/319001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.h (right):

https://codereview.chromium.org/26442004/diff/319001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.h:61: string16 message);
On 2013/10/25 21:41:25, michaeln wrote:
> s/b const string16& here

whoops. done.

https://codereview.chromium.org/26442004/diff/319001/content/common/service_w...
File content/common/service_worker_messages.h (right):

https://codereview.chromium.org/26442004/diff/319001/content/common/service_w...
content/common/service_worker_messages.h:50: // between registration and
unregistration errors.
On 2013/10/25 21:41:25, michaeln wrote:
> The first sentence looks good enough.

Done.

https://codereview.chromium.org/26442004/diff/319001/content/common/service_w...
content/common/service_worker_messages.h:55: string16 /* message */)
On 2013/10/25 21:41:25, michaeln wrote:
> should we use utf8 here and upsample at the WebKit API boundary?

It's fairly common to just use the enum and IPC_ENUM_TRAITS and I like the
explicit nature of it.
https://code.google.com/p/chromium/codesearch#search/&q=IPC_ENUM_TRAITS.*WebK...

[alecflett, 2013-10-25 23:45:35]

jam@ this one builds on http://crrev.com/25008006

[michaeln, 2013-10-26 00:41:00]

https://codereview.chromium.org/26442004/diff/319001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/26442004/diff/319001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:140: if (!callbacks)
On 2013/10/25 23:41:56, alecflett wrote:
> On 2013/10/25 21:41:25, michaeln wrote:
> > do we need the test for !callback?
> 
> We've needed to do this for security reasons in IDB - error checking on the
IPC
> boundary.

that makes good sense to me in the browser process, but less so in child
processes, i think we can trust completely ipc params going from browser->child,
and be very wary of param values going from child->browser

https://codereview.chromium.org/26442004/diff/319001/content/common/service_w...
File content/common/service_worker_messages.h (right):

https://codereview.chromium.org/26442004/diff/319001/content/common/service_w...
content/common/service_worker_messages.h:55: string16 /* message */)
On 2013/10/25 23:41:56, alecflett wrote:
> On 2013/10/25 21:41:25, michaeln wrote:
> > should we use utf8 here and upsample at the WebKit API boundary?
> 
> It's fairly common to just use the enum and IPC_ENUM_TRAITS and I like the
> explicit nature of it.

I was asking about the string value type?

We could use std::string here and rely on WebString::fromUTF8(stdstring). Would
take a little voodoo out of the simple case of returning a simple constant
string value message.

[michaeln, 2013-10-26 00:57:39]

https://codereview.chromium.org/26442004/diff/319001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/26442004/diff/319001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:60: if
(scope.GetOrigin() != script_url.GetOrigin())
On 2013/10/25 21:41:25, michaeln wrote:
> this can use the error return path now and probably should come after the
> enabled check

was wondering what happened to this one... i guess there is no value for
SecurityError defined yet, a generic Error value might help while we're fleshing
out the what errors we want specific values for... wdyt?

in any case, putting the enabled test up top seems like a good idea

https://codereview.chromium.org/26442004/diff/389001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/26442004/diff/389001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:22:
context_ = new ServiceWorkerContext(dir_.path(), NULL);
dir_ isn't needed now that there is no dir anymore

[alecflett, 2013-10-28 17:03:13]

ok, think I got all of 'em this time. I keep getting 500's uploading but for
once the patches seem to look fine

https://codereview.chromium.org/26442004/diff/319001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/26442004/diff/319001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:60: if
(scope.GetOrigin() != script_url.GetOrigin())
On 2013/10/26 00:57:39, michaeln wrote:
> On 2013/10/25 21:41:25, michaeln wrote:
> > this can use the error return path now and probably should come after the
> > enabled check
> 
> was wondering what happened to this one... i guess there is no value for
> SecurityError defined yet, a generic Error value might help while we're
fleshing
> out the what errors we want specific values for... wdyt?
> 
> in any case, putting the enabled test up top seems like a good idea 

oops, just an oversight. Done. (Add added a fairly generic "SecurityError" to
the WebKit side)

https://codereview.chromium.org/26442004/diff/319001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/26442004/diff/319001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:140: if (!callbacks)
On 2013/10/26 00:41:01, michaeln wrote:
> On 2013/10/25 23:41:56, alecflett wrote:
> > On 2013/10/25 21:41:25, michaeln wrote:
> > > do we need the test for !callback?
> > 
> > We've needed to do this for security reasons in IDB - error checking on the
> IPC
> > boundary.
> 
> that makes good sense to me in the browser process, but less so in child
> processes, i think we can trust completely ipc params going from
browser->child,
> and be very wary of param values going from child->browser
> 
Agreed that it's certainly much safer in the browser->child case, but I don't
think we can trust it completely.

But actually trust is just one issue: Going back and looking at IDB history I'm
also remembering that there were shutdown ordering issues that resulted in this
check: Some more details in http://crbug.com/104492

Long explanation: occasionally if a tab was closed while a transaction was
finishing, the renderer-side dispatcher could get cleaned up before the callback
finished. In certain cases, the IPC transaction-cancel message would get sent
from the renderer but wouldn't be serviced by the browser in time to avoid the
attempted callback. End result was a renderer crash.

I think that this addresses the same situation.

https://codereview.chromium.org/26442004/diff/319001/content/common/service_w...
File content/common/service_worker_messages.h (right):

https://codereview.chromium.org/26442004/diff/319001/content/common/service_w...
content/common/service_worker_messages.h:55: string16 /* message */)
On 2013/10/26 00:41:01, michaeln wrote:
> On 2013/10/25 23:41:56, alecflett wrote:
> > On 2013/10/25 21:41:25, michaeln wrote:
> > > should we use utf8 here and upsample at the WebKit API boundary?
> > 
> > It's fairly common to just use the enum and IPC_ENUM_TRAITS and I like the
> > explicit nature of it.
> 
> I was asking about the string value type?
> 
> We could use std::string here and rely on WebString::fromUTF8(stdstring).
Would
> take a little voodoo out of the simple case of returning a simple constant
> string value message.
> 

Oh I see. I personally prefer doing the conversion at the point (or at least in
the same process) as where the string is created...

https://codereview.chromium.org/26442004/diff/389001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/26442004/diff/389001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:22:
context_ = new ServiceWorkerContext(dir_.path(), NULL);
On 2013/10/26 00:57:40, michaeln wrote:
> dir_ isn't needed now that there is no dir anymore

Done.

[michaeln, 2013-10-28 20:46:35]

https://codereview.chromium.org/26442004/diff/319001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/26442004/diff/319001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:85:
g_dispatcher_tls.Pointer()->Set(NULL);
I think i see the potential reason for the !callbacks checks. In this class, the
only way in which request_id's would not be found is if we go down this
NOTREACHED re-initialization code path.

While we still can, it might be better to to return NULL here, instead of
opening the door to odd inputs and states, and to change the consumers to check
for NULL and do an early return if needed.

I'll make a CL for that.

[alecflett, 2013-10-30 02:00:10]

jam@ can I get a quick review for content stuff? (I think the only thing needing
owner review is the content*.gyp)

[jam, 2013-10-31 20:39:29]

lgtm

https://codereview.chromium.org/26442004/diff/479001/content/browser/service_...
File content/browser/service_worker/service_worker_context.h (right):

https://codereview.chromium.org/26442004/diff/479001/content/browser/service_...
content/browser/service_worker/service_worker_context.h:35: bool enabled();
nit: per style guide, this has to be capitalized since it's not inline. so
something like bool IsEnabled();

[commit-bot: I haz the power, 2013-11-04 22:10:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/26442004/619001

[commit-bot: I haz the power, 2013-11-05 00:42:13]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[alecflett, 2013-11-05 01:05:15]

abarth@ - can I get a quick lgtm because I'm adding
third_party/WebKit/platform/public/platform/WebServiceWorkerError.h to
content/browser ? (Needed for enums)

[commit-bot: I haz the power, 2013-11-05 04:27:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/26442004/619001

[commit-bot: I haz the power, 2013-11-05 05:07:03]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-05 15:37:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/26442004/619001

[commit-bot: I haz the power, 2013-11-05 15:57:55]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-06 01:23:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/26442004/619001

[commit-bot: I haz the power, 2013-11-06 02:26:14]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-06 16:35:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/26442004/1229001

[commit-bot: I haz the power, 2013-11-06 17:22:24]

Retried try job too often on linux_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-06 18:32:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/26442004/1469001

[commit-bot: I haz the power, 2013-11-06 23:43:12]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[alecflett, 2013-11-07 00:39:47]

Committed patchset #14 manually as r233438 (presubmit successful).