Closed Bug 1142852 Opened 5 years ago Closed 5 years ago

Cache Action should use non-threadsafe ref-counting

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
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.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Attachment #8577048 - Flags: review?(nfroyd)
Oops.  That was the wrong patch.
Attachment #8577048 - Attachment is obsolete: true
Attachment #8577048 - Flags: review?(nfroyd)
Attachment #8577049 - Flags: review?(nfroyd)
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.
Attachment #8577051 - Flags: review?(ehsan)
Attachment #8577049 - Attachment description: Bug 1142852 P1 Fix NS_NewNonOwningRunnableMethodWithArgs() so that it compiles. r=froyd nj → P1 Fix NS_NewNonOwningRunnableMethodWithArgs() so that it compiles. r=froyd nj
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().
Attachment #8577051 - Flags: review?(ehsan)
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.
Attachment #8577049 - Flags: review?(nfroyd) → review+
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().
Depends on: 1110487
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.
Attachment #8577051 - Flags: review?(ehsan)
Attachment #8577051 - Flags: review?(ehsan) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/07234a94ed48
https://hg.mozilla.org/mozilla-central/rev/2e9b6048bd0f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.