Possible GC/CC hole in Promise code

RESOLVED FIXED in Firefox 43

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla44
Points:
---

Firefox Tracking Flags

(firefox43+ fixed, firefox44+ fixed)

Details

Attachments

(1 attachment)

Consider the following sequence of events:

1)  Page does |new Promise()|
2)  We land in Promise::Constructor, create a Promise and a reflector for it,
    preserve the reflector.  Promise is owned by nsRefPtr on the stack.
3)  We call Promise::CallInitFunction which calls Promise::CreateFunction.
4)  CreateFunction calls NewFunctionWithReserved which does a GC before or after
    creating the object.  This marks the Promise reflector gray, since the only
    thing owning it is the C++ Promise object.
5)  We call GetOrCreateDOMreflector to stash the Promise reflector into a
    Rooted<Value>.  This does NOT unmarkgray.
6)  We put the value in a reserved slot on the JSFunction.  Now we have a white
    or black object pointing to a gray object, as far as Andrew and I can tell.
7)  We unwind back out into JS, returning the Promise reflector.  As far as I
    can tell it's still marked gray.
8)  We now perform a CC (before the next GC!).  The Promise reflector is gray,
    there are no C++ references to the Promise, so the Promise gets unlinked, if
    I understand this stuff correctly.
9)  Now if someone calls the function that has the Promise reflector in its
    reserved slot, we get the situation of bug 1182197.

Seem plausible?
The black-gray edge is being created in step 6, which I think is the call to js::SetFunctionNativeReserved() at the end of Promise::CreateFunction(). We should consider adding some kind of write barrier to js::SetFunctionNativeReserved().  ie unmark gray the value if the object you are writing to is non-gray.
Blocks: 1182197
Created attachment 8672068 [details] [diff] [review]
Unmark gray things before putting them into function reserved slots in Promise code
Attachment #8672068 - Flags: review?(continuation)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8672068 [details] [diff] [review]
Unmark gray things before putting them into function reserved slots in Promise code

Approval Request Comment
[Feature/regressing bug #]: Unknown
[User impact if declined]: Probably crashes as in bug 1182197
[Describe test coverage new/current, TreeHerder]: we use promises.  I can't say
   more than that, because this stuff is so gc/cc timing dependent.
[Risks and why]: I think this is very very low risk.
[String/UUID change made/needed]: None.
Attachment #8672068 - Flags: approval-mozilla-aurora?
Comment on attachment 8672068 [details] [diff] [review]
Unmark gray things before putting them into function reserved slots in Promise code

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

This looks good as something to backport, though ideally we'd fix this in a less fragile fashion.
Attachment #8672068 - Flags: review?(continuation) → review+
I filed bug 1213401 on fixing js::SetFunctionNativeReserved().

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f65467c4aa61
https://hg.mozilla.org/mozilla-central/rev/f65467c4aa61
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment on attachment 8672068 [details] [diff] [review]
Unmark gray things before putting them into function reserved slots in Promise code

Speculative fix that may prevent GC crashes, ok to uplift to aurora.
Attachment #8672068 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assuming this affects 43, well, since bz says so. And, I'm tracking this to make sure the uplift goes ok.
status-firefox43: --- → affected
tracking-firefox43: --- → +
tracking-firefox44: --- → +
https://hg.mozilla.org/releases/mozilla-aurora/rev/369aac182bf6
status-firefox43: affected → fixed
Sorry had to backout and recheckin this since this turned innocent for a memory leaks, sorry for the hassle
You need to log in before you can comment on or make changes to this bug.