Start EmbeddedWorker during registration - take 2

Start EmbeddedWorker during registration - take 2

This adds the starting of the server worker to the registration
job pipeline. Eventually this will also dispatch the 'install'
event.

A second version of this, based on kinuko's work

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

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

[alecflett, 2014-01-18 01:08:49]

Here I've taken http://crrev.com/139093002 and rebased it on top of
http://crrev.com/141413004

kinuko, your start/stop observer classes were way more robust, and this
appropriately deals with the new ServiceWorkerVersionStatus - though you should
look at the slightly ugly switch() statement in service_worker_register_job.cc

This is still really ugly with lots of debug statements and whatnot (and a
really ugly callback declaration in service_worker_register_job.cc) - and it
isn't managing the reference to ServiceWorkerProviderHost at all. I *think* that
it should be a weak pointer.

I think after your stuff lands, I'd like to see how this is looking

[kinuko, 2014-01-20 09:39:08]

I see, your patch seems to have exposed a few important missing parts in the
current architecture.

https://codereview.chromium.org/140743012/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/140743012/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_register_job.cc:42:
pending_version_->OnAssociateProvider(source_provider);
How do we relate this to ServiceWorkerRegistration's pending_ and
active_version, and ProviderHost's associated_version?

I think it'd be probably better to first clear up how ProviderHost -
Registration - Version should be connected with each other (while StartWorker
part is being blocked by my other patch).

I assume:
- Provider / ProviderHost's created per document and tracks what URL it's
rendering, and
- Provider / ProviderHost is associated with a particular ServiceWorkerVersion
if its document URL matches with a registration

And currently we don't have either part. Is it possible to plumb this part
first?

Also I assumed we'd like to run EW on a process that renders a document whose
navigator.serviceWorker is associated with the corresponding
ServiceWorkerVersion, but on the second thought it doesn't need to use the
current association information, we could just run it on any process that
belongs to the same origin (using the document URL information in SWPH)?
--otherwise we'll have a bootstrapping problem as this patch has exposed.

https://codereview.chromium.org/140743012/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_register_job.cc:160:
pending_version_->OnAssociateProvider(*it);
As I wrote above I feel this association should be probably done in two parts:

- for EW process assignment, which should be wired probably outside this
registration job
- for ServiceWorkerProviderHost - ServiceWorkerVersion association, which should
be done after installation/activation is done and a particular version is
associated to the ServiceWorkerProvider(Host). It'll going to be used to serve
onfetch() event for the document

[kinuko, 2014-01-28 06:34:55]

https://codereview.chromium.org/140743012/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/140743012/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_register_job.cc:84:
register_new));
While revisiting the installation flow this just caught me-- we always
unregister registration (which unrefs the old registration in in-memory store in
the current code) before installing a new one, but if so how do we maintain the
'pending' and 'active' version for the same registration?

ServiceWorkerRegistration has active_version_ and pending_version_ fields, so it
seems it's supposed to keep both while update is ongoing?

[alecflett, 2014-01-28 23:19:42]

On 2014/01/28 06:34:55, kinuko wrote:
>
https://codereview.chromium.org/140743012/diff/1/content/browser/service_work...
> File content/browser/service_worker/service_worker_register_job.cc (right):
> 
>
https://codereview.chromium.org/140743012/diff/1/content/browser/service_work...
> content/browser/service_worker/service_worker_register_job.cc:84:
> register_new));
> While revisiting the installation flow this just caught me-- we always
> unregister registration (which unrefs the old registration in in-memory store
in
> the current code) before installing a new one, but if so how do we maintain
the
> 'pending' and 'active' version for the same registration?
> 
> ServiceWorkerRegistration has active_version_ and pending_version_ fields, so
it
> seems it's supposed to keep both while update is ongoing?

That if() statement is a little complex - we only unregister if the script url
is different.

I have an updated patch (still not ready for review, but will help clarify this)

[alecflett, 2014-01-29 01:22:58]

Ok new patch is up. the basic outstanding issues are excessive log messages
(from my debugging) and the fact that existing tests are failing. (I need to
switch the job_unittest over to use EmbeddedWorkerTestHelper)

the basic simplification that this does is that we start the worker, and set it
active simultaneously, bypassing the install and activate events entirely.

kinuko, once your various install patches land, I can hook up to that. I'd like
to be able to dispatch the install event to the worker whether or not it's
actually started though - and have the worker start if it needs to start. that
way the registration job doesn't have to worry about starting and restarting the
worker if the install event fails for any reason/etc.

[kinuko, 2014-01-29 02:21:30]

On 2014/01/29 01:22:58, alecflett wrote:
> Ok new patch is up. the basic outstanding issues are excessive log messages
> (from my debugging) and the fact that existing tests are failing. (I need to
> switch the job_unittest over to use EmbeddedWorkerTestHelper)
> 
> the basic simplification that this does is that we start the worker, and set
it
> active simultaneously, bypassing the install and activate events entirely.
>
> kinuko, once your various install patches land, I can hook up to that. I'd
like
> to be able to dispatch the install event to the worker whether or not it's
> actually started though - and have the worker start if it needs to start. that
> way the registration job doesn't have to worry about starting and restarting
the
> worker if the install event fails for any reason/etc.

Ok, then I can make each of DispatchInstallEvent(), DispatchFetchEvent() etc
method of ServiceWorkerVersion ensure that the worker's actually started before
sending the message, so that the consumer can directly send Dispatch*() method
without worrying about the worker's running status.  Does it make sense?

[kinuko, 2014-01-29 09:56:20]

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance.cc:21: LOG(ERROR) <<
"Starting EmbeddedWorkerInstance";
You could add these logs with DVLOG(1) so that we can keep useful logging in the
debug build

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance.cc:24: LOG(ERROR) <<
"Couldnl't find process to run worker..";
I'm changing StartWorker() to return PROCESS_NOT_FOUND error code so that you
could use it...

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.h:73: int
source_process_id,
I'm not fully sure yet if passing process_id here is the right thing, but during
the registration flow it'd be probably most convenient (at least for now..)

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
File content/browser/service_worker/service_worker_context_unittest.cc (right):

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_context_unittest.cc:62:
embedded_worker_id_ = worker->embedded_worker_id();
These lines for embedded_worker_id_ are not necessary?

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

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:120: }
Do we need this check?

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_job_unittest.cc:109: &sink_);
Some lines (setting up its own registry etc) seem to be a remnant of the old
code?

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:106:
callback.Run(registration, SERVICE_WORKER_OK);
This is going to dispatch install event when it's wired?

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:111: registration,
worker_registry_, registration->next_version_id());
Oh, is a version ID going to be a registration-local?

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:121:
registration->set_active_version(pending_version_);
This should be set_pending_version() until we complete Install, shouldn't it?

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:168: // schedule
an update.
This comment seems to say more specific situation than the cases that can fall
into this 'else'

https://codereview.chromium.org/140743012/diff/50001/content/common/service_w...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/140743012/diff/50001/content/common/service_w...
content/common/service_worker/service_worker_messages.h:32: int /* provider_id
*/,
It looks we don't need to send this back to browser?

[alecflett, 2014-01-30 01:51:12]

ok, still not done here - I was able to fix a lot of tests by switching over
completely to EmbeddedWorkerTestHelper, but I still have 2 failing tests.

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance.cc:21: LOG(ERROR) <<
"Starting EmbeddedWorkerInstance";
On 2014/01/29 09:56:20, kinuko wrote:
> You could add these logs with DVLOG(1) so that we can keep useful logging in
the
> debug build

I don't think these are super useful, all of the LOG(ERROR)'s will be going
away... they're mostly for when I was debugging test failures.

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance.cc:24: LOG(ERROR) <<
"Couldnl't find process to run worker..";
On 2014/01/29 09:56:20, kinuko wrote:
> I'm changing StartWorker() to return PROCESS_NOT_FOUND error code so that you
> could use it...

Yep, will do!

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.h:73: int
source_process_id,
On 2014/01/29 09:56:20, kinuko wrote:
> I'm not fully sure yet if passing process_id here is the right thing, but
during
> the registration flow it'd be probably most convenient (at least for now..)

Yeah - I mean at least passing the ServiceWorkerProviderHost gave a hint as for
what this is for.. but for reduced dependencies I think process id will work
here.

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
File content/browser/service_worker/service_worker_context_unittest.cc (right):

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_context_unittest.cc:62:
embedded_worker_id_ = worker->embedded_worker_id();
On 2014/01/29 09:56:20, kinuko wrote:
> These lines for embedded_worker_id_ are not necessary?

They are for the code as it is now - where StartWorker is part of the install
process - in fact that's part of the point of passing process_id in elsewhere -
otherwise the EmbeddedWorkerRegistry is unaware of the process_id that gets
passed in.

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

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:120: }
On 2014/01/29 09:56:20, kinuko wrote:
> Do we need this check?

I guess we don't now! (an earlier iteration needed it)

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_job_unittest.cc:109: &sink_);
On 2014/01/29 09:56:20, kinuko wrote:
> Some lines (setting up its own registry etc) seem to be a remnant of the old
> code?

yep sorry I didn't mean you had to review this one, this test was broken when I
sent this.

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:111: registration,
worker_registry_, registration->next_version_id());
On 2014/01/29 09:56:20, kinuko wrote:
> Oh, is a version ID going to be a registration-local?

I don't have strong feelings about it either way.

https://codereview.chromium.org/140743012/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:121:
registration->set_active_version(pending_version_);
On 2014/01/29 09:56:20, kinuko wrote:
> This should be set_pending_version() until we complete Install, shouldn't it?

in the final code yes - in the short term (until all the install event stuff
lands) I'm just skipping the pending state.

https://codereview.chromium.org/140743012/diff/50001/content/common/service_w...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/140743012/diff/50001/content/common/service_w...
content/common/service_worker/service_worker_messages.h:32: int /* provider_id
*/,
On 2014/01/29 09:56:20, kinuko wrote:
> It looks we don't need to send this back to browser?

I suppose we don't now.

[alecflett, 2014-02-04 00:14:32]

this patch has been rebased on top of http://crrev.com/150443002 

I had to change one assumption - in embedded_worker_registry made the map of
renderer_process_id -> embedded_worker_id a map<int> because I had a test that
created two independent service workers in the same child process.

This patch is ALMOST ready for full review - just need to clean up the excessive
logging.

[alecflett, 2014-02-04 01:14:13]

ok, this patch is ready to go, finally.

As it mentions, we're skipping the install/activate step and going right to
setting a worker as 'active'.

[kinuko, 2014-02-04 03:37:52]

Ok, let's land this and hack on it. lgtm

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
File content/browser/service_worker/embedded_worker_registry.cc (right):

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
content/browser/service_worker/embedded_worker_registry.cc:94: if (found !=
worker_process_map_.end()) {
nit: let's assign worker_process_map_[process_id] to a temp variable before the
for loop

const std::set<int>& worker_set = worker_process_map_[process_id];

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
File content/browser/service_worker/embedded_worker_registry.h (right):

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
content/browser/service_worker/embedded_worker_registry.h:88: std::map<int,
std::set<int> > worker_process_map_;
gotcha, thx for fixing

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_context_unittest.cc (right):

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_context_unittest.cc:61:
helper_->SimulateAddProcess(embedded_worker_id, render_process_id_);
(Oh I see... looks like the helper's still a bit inconvenient for this type of
testing, I think I can make a follow-up patch to cleanup some of these)

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:128:
EmbeddedWorkerMsg_StartWorker::ID));
Could we have one more GetUniqueMessageMatching for the other message? (Since we
assert it gets two messages)

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_job_unittest.cc:124:
ServiceWorkerJobCoordinator* job_coordinator_;
nit: could we instead have accessors like storage() and job_coordinator() ?

(If you prefer having these fields please initialize them with NULL in ctor)

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_job_unittest.cc:126: int
render_process_id_;
Please initialize this in ctor (with -1 etc)

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:52: LOG(ERROR) <<
"Starting job of type " << type_;
Please remove this line (or replace with DVLOG) before submit

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:113: LOG(ERROR) <<
"StartWorkerAndContinue...";
Ditto, please remove or replace with DVLOG

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:122: // callback
has executed.
After my next a few patches land (if they look good to you) you'll just be able
to call pending_version_->DispatchInstallEvent() and it'll automatically start
the worker if necessary.

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:161: <<
previous_status << ", " << previous_registration << ");";
ditto

[kinuko, 2014-02-04 11:41:31]

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_context_unittest.cc (right):

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_context_unittest.cc:61:
helper_->SimulateAddProcess(embedded_worker_id, render_process_id_);
Btw you can just remove line 56-61 and instead add a following line to add a
sender, which would be probably cleaner:

context_->embedded_worker_registry()->AddChildProcessSender(
    render_process_id_, helper_.get());

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:40:
helper_->SimulateAddProcess(embedded_worker_id, render_process_id);
Ditto. You can remove line 35-40 and add:

    context()->embedded_worker_registry()->AddChildProcessSender(
        kRenderProcessId, helper_.get());

[alecflett, 2014-02-04 20:08:19]

The CQ bit was checked by alecflett@chromium.org

[alecflett, 2014-02-04 20:09:04]

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
File content/browser/service_worker/embedded_worker_registry.cc (right):

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
content/browser/service_worker/embedded_worker_registry.cc:94: if (found !=
worker_process_map_.end()) {
On 2014/02/04 03:37:53, kinuko wrote:
> nit: let's assign worker_process_map_[process_id] to a temp variable before
the
> for loop
> 
> const std::set<int>& worker_set = worker_process_map_[process_id];

Done.

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_context_unittest.cc (right):

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_context_unittest.cc:61:
helper_->SimulateAddProcess(embedded_worker_id, render_process_id_);
On 2014/02/04 11:41:31, kinuko wrote:
> Btw you can just remove line 56-61 and instead add a following line to add a
> sender, which would be probably cleaner:
> 
> context_->embedded_worker_registry()->AddChildProcessSender(
>     render_process_id_, helper_.get());

I ended up adding a helper to embedded_worker_test_helper - I was concerned that
SimulateAddProcess could get more complex.

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:40:
helper_->SimulateAddProcess(embedded_worker_id, render_process_id);
On 2014/02/04 11:41:31, kinuko wrote:
> Ditto. You can remove line 35-40 and add:
> 
>     context()->embedded_worker_registry()->AddChildProcessSender(
>         kRenderProcessId, helper_.get());

Done.

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:128:
EmbeddedWorkerMsg_StartWorker::ID));
On 2014/02/04 03:37:53, kinuko wrote:
> Could we have one more GetUniqueMessageMatching for the other message? (Since
we
> assert it gets two messages)

Done.

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_job_unittest.cc:124:
ServiceWorkerJobCoordinator* job_coordinator_;
On 2014/02/04 03:37:53, kinuko wrote:
> nit: could we instead have accessors like storage() and job_coordinator() ?
> 
> (If you prefer having these fields please initialize them with NULL in ctor)

Done.

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_job_unittest.cc:126: int
render_process_id_;
On 2014/02/04 03:37:53, kinuko wrote:
> Please initialize this in ctor (with -1 etc)

Done.

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/140743012/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:52: LOG(ERROR) <<
"Starting job of type " << type_;
On 2014/02/04 03:37:53, kinuko wrote:
> Please remove this line (or replace with DVLOG) before submit

Done.

[commit-bot: I haz the power, 2014-02-04 20:14:19]

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

[commit-bot: I haz the power, 2014-02-04 21:14:41]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-02-04 22:32:50]

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

[commit-bot: I haz the power, 2014-02-05 01:12:27]

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

[commit-bot: I haz the power, 2014-02-05 06:05:02]

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

[commit-bot: I haz the power, 2014-02-05 06:05:02]

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

[alecflett, 2014-02-05 18:00:14]

The CQ bit was checked by alecflett@chromium.org

[commit-bot: I haz the power, 2014-02-05 18:09:40]

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

[commit-bot: I haz the power, 2014-02-05 20:55:22]

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

[commit-bot: I haz the power, 2014-02-05 20:55:23]

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

[alecflett, 2014-02-05 23:02:03]

The CQ bit was checked by alecflett@chromium.org

[commit-bot: I haz the power, 2014-02-05 23:04:43]

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

[alecflett, 2014-02-06 00:28:36]

I was going to manually commit until I saw that
ServiceWorkerVersionBrowserTest.StartNotFound is failing on linux-aura, with
assertions  like DCHECK(jobs_.empty())

I can't reproduce that locally though...

[commit-bot: I haz the power, 2014-02-06 00:44:28]

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

[commit-bot: I haz the power, 2014-02-06 00:44:28]

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

[kinuko, 2014-02-06 08:43:35]

On 2014/02/06 00:44:28, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_chromeos for step(s) content_browsertests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

I know why it's failing... in the content_browsertests we first load
content/test/data/service_worker/index.html which kicks registerServiceWorker
(which enqueues a new registration job, though it used to do almost nothing in
the past). I think I can change how the current browsertest works

[alecflett, 2014-02-06 19:03:48]

On 2014/02/06 08:43:35, kinuko wrote:
> On 2014/02/06 00:44:28, I haz the power (commit-bot) wrote:
> > Retried try job too often on linux_chromeos for step(s) content_browsertests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
> 
> I know why it's failing... in the content_browsertests we first load
> content/test/data/service_worker/index.html which kicks registerServiceWorker
> (which enqueues a new registration job, though it used to do almost nothing in
> the past). I think I can change how the current browsertest works

Yeah, I figured it was something like that but I really cant reproduce it on my
machine.. I'll keep trying.

[alecflett, 2014-02-06 23:28:01]

The CQ bit was checked by alecflett@chromium.org

[commit-bot: I haz the power, 2014-02-06 23:29:07]

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

[commit-bot: I haz the power, 2014-02-07 00:09:22]

Change committed as 249544

[alph, 2014-02-07 11:44:34]

A revert of this CL has been created in
https://codereview.chromium.org/137233019/ by alph@chromium.org.

The reason for reverting is: Makes
virtual/serviceworker/http/tests/serviceworker/registration.html crash.
See
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/26615.

[kinuko, 2014-02-07 13:59:04]

Alec: please see comments below for the crash cause in chromium side.

We also need fixes in blink side-- I uploaded 2 patches for it
(https://codereview.chromium.org/157273005/ and
https://codereview.chromium.org/152883004/)

https://codereview.chromium.org/140743012/diff/540001/content/browser/service...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/140743012/diff/540001/content/browser/service...
content/browser/service_worker/service_worker_context_core.cc:100:
DCHECK(!registration);
This could fail (crash in debug build) unless we handle error case differently
in ServiceWorkerRegisterJob::RegisterComplete() (please see the other comment)

https://codereview.chromium.org/140743012/diff/540001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/140743012/diff/540001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:188:
RunCallbacks(start_status, registration);
Looks like we should handle error case here (e.g. unregister the registration
and pass an empty registration etc)

[alecflett, 2014-02-07 22:20:56]

The CQ bit was checked by alecflett@chromium.org

[commit-bot: I haz the power, 2014-02-07 22:34:11]

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

[commit-bot: I haz the power, 2014-02-07 22:40:20]

Change committed as 249828