Ben Kelly [:bkelly, not reviewing] Assignee
	Description • 9 years ago

In P4 from bug 1110485 I attempted to keep any Worker alive during Cache async operations.  This means ignoring WorkerFeature::Notify() calls during these operations.

The error in bug 1134841 suggests that I might have missed a few cases.  In particular, the CachePushStreamChild still shuts down immediately.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 1 • 9 years ago

This patch improves the Cache handling of the WorkerFeature shutdown in a number of ways:

1) Check for mActor==nullptr in Cache.cpp since its conceivable that JS could call Cache methods after WorkerFeature::Notify() kills the actor.
2) Do not abort the CachePushStreamChild actor on WorkerFeature::Notify().  We want to keep the worker alive until all async operations complete normally.
3) Make async operations hold the Cache and CacheStorage objects alive.  This means we can keep the actor attached to the DOM objects while async ops are in flight.  This in turn limits the chance script will see an NS_ERROR_UNEXPECTED because the actor died.
4) Only start destruction from the WorkerFeature when the Canceling state is reached.  This also limits the chance of content seeing an NS_ERROR_UNEXPECTED.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57aaa2d727c7

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 2 • 9 years ago

Comment on attachment 8599907 [details] [diff] [review]
Improve Cache API WorkerFeature shutdown handling. r=baku

I see a typo I should fix.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 3 • 9 years ago

Updated with fix.  See comment 1 for description of the overall changes in the patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d13d430eb2a

	Andrea Marchesini [:baku]
	Comment 4 • 9 years ago

Comment on attachment 8599911 [details] [diff] [review]
Improve Cache API WorkerFeature shutdown handling. r=baku

Review of attachment 8599911 [details] [diff] [review]:
-----------------------------------------------------------------

The rest is good. Can you give me a feedback about the use of mDelayedDestroy?

::: dom/cache/CacheChild.cpp
@@ +175,5 @@
>  CacheChild::NoteDeletedActor()
>  {
>    mNumChildActors -= 1;
> +  if (!mNumChildActors && mDelayedDestroy) {
> +    StartDestroy();

Same issue here.

::: dom/cache/CacheStorageChild.cpp
@@ +135,5 @@
>  CacheStorageChild::NoteDeletedActor()
>  {
>    MOZ_ASSERT(mNumChildActors);
>    mNumChildActors -= 1;
> +  if (!mNumChildActors && mDelayedDestroy) {

it seems to me that StartDestroy() is never called:

mDelayedDestroy is false by default and it's set to true by StartDestroy()
Bu StartDestroy is called only if mDelayedDestroy is true.

Am I missing something?

	Andrea Marchesini [:baku]
	Comment 5 • 9 years ago

Comment on attachment 8599911 [details] [diff] [review]
Improve Cache API WorkerFeature shutdown handling. r=baku

Review of attachment 8599911 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. Ignore the previous comments. Thanks!

	Pulsebot
	Comment 6 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8ada80625de

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 7 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/c8ada80625de