Closed Bug 1213391 Opened 5 years ago Closed 5 years ago
Possible GC/CC hole in Promise code
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.
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 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
You need to log in before you can comment on or make changes to this bug.