Possible GC/CC hole in Promise code

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

Trunk
mozilla44
Points:
---
Dependency tree / graph

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
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().
https://hg.mozilla.org/mozilla-central/rev/f65467c4aa61
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
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.
Sorry had to backout and recheckin this since this turned innocent for a memory leaks, sorry for the hassle
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.