1142852 - Cache Action should use non-threadsafe ref-counting

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

While looking at bug 1142803 I noticed that Action uses threadsafe ref-counting.  This is not strictly correct as we require these objects to survive until they complete on their owning thread.  It would be much safer to simply forbid Release()'ing Actions off their owning thread.

Changing Action back to non-threadsafe works for most of the Actions without difficulty.  The only problematic Action is that async CachePutAllAction.  It AddRef()'s on the IO thread while performing stream copying.  This is not necessary, though, and can be replaced with non-owning weak refs due to our Action life cycle guarantees.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Fix NS_NewNonOwningRunnableMethodWithArgs() so that it compiles. r=froydnj

NS_NewNonOwningRunnableMethodWithArgs() exists in the tree, but is not used.  As a result it has a typo and won't compile if you try to use it.  This fixes the typo.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Fix NS_NewNonOwningRunnableMethodWithArgs() so that it compiles. r=froyd nj

Oops.  That was the wrong patch.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make Cache Action use non-threadsafe ref-counting. r=ehsan

This makes Action use non-threadsafe ref-counting.  It requires CachePutAllAction to depend on our life cycle guarantees so that it can use non-owning references.  I think that is safer than possibly allowing a destructor on a separate thread when we must have completion on the owning thread.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8577051 [details] [diff] [review]
P2 Make Cache Action use non-threadsafe ref-counting. r=ehsan

Actually, I'm not sure this is advisable.  After sleeping on it I now remember that we need to hold a strong ref in the async action case to prevent the object from being destroyed out from under itself when it calls Resolve().

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8577049 [details] [diff] [review]
P1 Fix NS_NewNonOwningRunnableMethodWithArgs() so that it compiles. r=froyd nj

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

Doh.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

I think this will be safe if we land the P3 patch from bug 1110487.  That ensures that the Action does not get deleted out from under the method that calls Resolve().

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8577051 [details] [diff] [review]
P2 Make Cache Action use non-threadsafe ref-counting. r=ehsan

The changes to Context I've proposed in bug 1110487 allow us to tighten the constrains on Action.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

I included this one in some larger try builds I did:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b2ef58ad79e

The devtools failures are unrelated to these patches.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/07234a94ed48
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2e9b6048bd0f

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/07234a94ed48
https://hg.mozilla.org/mozilla-central/rev/2e9b6048bd0f