Summary: | ScriptExecutionContext::stopActiveDOMObjects iterates a hash map that can change during iteration (for multiple reasons, including GC) | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jeremy Orlow <jorlow> | ||||||
Component: | WebCore JavaScript | Assignee: | Darin Adler <darin> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, darin, esprehn+autocc, ggaren, kangil.han | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Jeremy Orlow
2011-01-19 06:43:16 PST
So, here is the bug. ScriptExecutionContext::stopActiveDOMObjects() iterates over a HashMap of ActiveDOMObjects, naturally expecting that the HashMap doesn't change.
However, stop() can reduce the reference count of the object, and there is no strong guarantee that it won't be deleted. For XMLHttpRequest in particular, it's possible that the only remaining reference is kept by its JS wrapper - and since the wrapper is now unprotected, garbage collection can destroy it! And then, the only protection is that JavaScriptCore GC doesn't actually delete objects until something is allocated in their place.
So, calling reportExtraMemoryCost() from XMLHttpRequest::stop() is particularly dangerous, because it can start garbage collection, which will delete previously unprotected active DOM objects. I'm less sure now that we should actually fix XMLHttpRequest, and not unsafe iteration.
> [7:56pm] jorlow: (though we should put in the ASSERTs you suggested)
In fact, HashMap already has assertions for invalidated iterators, which should fail if an iterator is used after its map has changed.
We should try to add stronger assertions though (that JS code doesn't run, or that GC isn't triggered). > I'm less sure now that we should actually fix XMLHttpRequest, and not unsafe iteration.
I think the right solution is to fix unsafe iteration. The simplest solution is probably to copy the hash map to a vector of RefPtr before iterating.
It's very difficult to establish a "nothing must happen" point in WebCore -- no JS execution, no GC, etc. And even if you did establish such a thing, it would be very difficult to test all the edge cases and make sure it worked.
Plus, I don't think we want to lose the memory footprint optimization of reporting extra cost once an XHR becomes orphaned.
The problem is that we still need to require no JS execution - because JS could add more active objects. So, guarding only against object destruction but not creation would be inconsistent. Also, there's a technical issue with ActiveDOMObjects not being refcounted themselves, so you can't just make a Vector<RefPtr<ActiveDOMObject> >. (In reply to comment #4) How about this, for solving the iteration problem: while (x = take(begin())) x->stop(); Then we'd have a freezing problem with code that actually runs JS and adds new ActiveDOMObjects. I don't think there is any safe way to iterate without imposing any restrictions on code that runs during iteration - the question is what requirements we can practically enforce in this particular case. (In reply to comment #1) > In fact, HashMap already has assertions for invalidated iterators, which should fail if an iterator is used after its map has changed. Sure, but we should also assert that we're not in the middle of iterating when we add/remove an object. Asserting later keeps us from doing something silly, but doesn't make it easy to see what the problem is. (In reply to comment #4) > The problem is that we still need to require no JS execution - because JS could add more active objects. So, guarding only against object destruction but not creation would be inconsistent. FWIW, I'm not sure this is such a big deal. (In reply to comment #5) > (In reply to comment #4) > How about this, for solving the iteration problem: > while (x = take(begin())) > x->stop(); That won't work because often need to iterate without destroying the list. In https://bugs.webkit.org/show_bug.cgi?id=52366 I had a solution that copied to a vector and double checked each element was still in the map before calling stop. I wasn't super happy with it, but it might be the best option if we want to support ActiveDOMObjects being destroyed during iteration. I tried hacking around the ActiveDOMObject not being ref counted problem in several different ways, but none of them were even close to clean enough that I think they'd be acceptable. We don’t need to allow arbitrary modification of the hash map, but we do need to allow removal from the map during the stopActiveDOMObject process. That’s the only practical problem. Maybe later we can make an even better model, but this will shore things up for now. (In reply to comment #7) > In https://bugs.webkit.org/show_bug.cgi?id=52366 I had a solution that copied to a vector and double checked each element was still in the map before calling stop. I wasn't super happy with it, but it might be the best option if we want to support ActiveDOMObjects being destroyed during iteration. Yes, I think that is indeed what we want. Created attachment 229757 [details]
Patch
Comment on attachment 229757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229757&action=review > Source/WebCore/ChangeLog:61 > + (WebCore::ScriptExecutionContext::didCreateActiveDOMObject): Ditto. Got rid of the > + CRASH because there's no compelling value to this being a release assert instead of a > + normal assert. Actually, I think that this release assert is critically important. It is very very bad to have unstopped active dom objects after stopActiveDOMObjects(). The idea of this patch looks right to me, but Id like to have another look when it's not so late at night. It's also worth documenting the restrictions on ActiveDOMObjects somewhere more persistent - the fact that stop() is allowed to delete the object, but is not allowed to execute arbitrary JS is not obvious. Also, what is allowed in suspend() or resume()? > Source/WebCore/dom/ScriptExecutionContext.cpp:279 > + for (auto* object : objects) { So the usual argument for why auto is good is that variable name has all the information a type name has. It's hardly possible to say less than "object" says! Created attachment 229767 [details]
Patch
Comment on attachment 229767 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229767&action=review r=me. Nice patch! > Source/WebCore/ChangeLog:61 > + (WebCore::ScriptExecutionContext::didCreateActiveDOMObject): Ditto. Got rid of the > + CRASH because there's no compelling value to this being a release assert instead of a > + normal assert. This comment is now obsolete. > Source/WebCore/ChangeLog:84 > + * workers/WorkerGlobalScope.h: Make hasPendingActivity function from the base class > + public instead of declaring it in this class. Longer term, I'd like to come up with a better name for it, it's quite unclear which "pending activity" we are talking about. The workers spec has changed a lot, it might have a better name and/or design for this concept now. > Source/WebCore/dom/ScriptExecutionContext.cpp:188 > + // No protection against m_activeDOMObjects changing during iteration: canSuspend() > + // functions should not add new active DOM objects, nor execute arbitrary JavaScript. The reworded comment doesn't seem quite accurate, as removing active DOM objects isn't OK either (and there is a debug only assertion for that). I thought that we had a way to assert that JS was not executed, but I can't find it now. I only found ScriptExecutionContext::isJSExecutionForbidden(), which is a different thing, and seems like a misnomer. It would be very useful to catch mistakes with unexpected JS execution sooner, not just when the script does something bad. > Source/WebCore/dom/ScriptExecutionContext.cpp:221 > + // No protection against m_activeDOMObjects changing during iteration: suspend() > + // functions should not add new active DOM objects, nor execute arbitrary JavaScript. Ditto. > Source/WebCore/dom/ScriptExecutionContext.cpp:249 > + // No protection against m_activeDOMObjects changing during iteration: resume() > + // functions should not add new active DOM objects, nor execute arbitrary JavaScript. Ditto. > Source/WebCore/dom/ScriptExecutionContext.cpp:277 > + for (auto* object : objects) { ... > Source/WebCore/dom/ScriptExecutionContext.cpp:313 > + // The m_activeDOMObjectAdditionForbidden check is a RELEASE_ASSERT because of the > + // consequences of having an ActiveDOMObject that is not correctly reflected in the set. > + // If we do have one of those, it can possibly be a security vulnerability. So we'd > + // rather have a crash than continue running with the set possibly compromised. suspendActiveDOMObjectIfNeeded() mechanism makes this almost unnecessary, because even objects that are created during iteration will be created suspended or stopped. But suspendActiveDOMObjectIfNeeded() itself is very fragile - every constructor has to call suspendIfNeeded() for it to work - so it's best to keep this protection I think. Comment on attachment 229767 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229767&action=review > Source/WebCore/dom/ScriptExecutionContext.cpp:109 > +// FIXME: Should we make this a member function of HashSet? > +// Which name is best: takeOne, takeFirst, or takeRandom? "takeAny"? I like "takeOne" too. "Random" is not so good because the results are not guaranteed to be random, so you'd be mistaken to use this function in a context like shuffling or ASLR. "First" implies some kind of order, which is not quite true of a hash table. Member function or not, I think it would be nice to put this function in HashSet.h, so readers of that header will see that you can do this sort of thing. (In reply to comment #13) > I thought that we had a way to assert that JS was not executed, but I can't find it now. I remember that too, but I also can’t find it. > It would be very useful to catch mistakes with unexpected JS execution sooner, not just when the script does something bad. I agree. I wonder what happened to it? Lets add it. I considered your comments and made a few additions to the patch, including comments in ActiveDOMObject.h. Committed r167579: <http://trac.webkit.org/changeset/167579> |