| Summary: | ScriptController::updateDocument ASSERT mutating map while iterating map | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||
| Component: | WebCore JavaScript | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ap, buildbot, commit-queue, ggaren, joepeck, kling, oliver, rniwa, sam | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Joseph Pecoraro
2014-07-23 14:30:56 PDT
> We should be able to work around this in the same way that did by not iterating over the live map.
Actually, this wants both the key and the value. =/
Can we just copy and iterate the copy? (In reply to comment #1) > > We should be able to work around this in the same way that did by not iterating over the live map. > > Actually, this wants both the key and the value. =/ We can get the value (DOMWrapperWorld) from the value (JSDOMWindowShell) JSDOMWindowShell::world, so copyValuesToVector is fine. (In reply to comment #2) > Can we just copy and iterate the copy? Looks like we can, but that also seems like it would do more work. Would that be preferred? Created attachment 235386 [details]
[PATCH] Proposed Fix
Saw no ASSERTs running "fast" tests with a debug WebKit. I'll do some more local testing experimenting with JSC_slowPathAllocsBetweenGCs.
Comment on attachment 235386 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=235386&action=review > Source/WebCore/ChangeLog:10 > + Avoid iterating over m_windowShells in more places. This prevents > + the possibility of a collection during JSC allocation which might > + cause a mutation to m_windowShells (HTMLMediaElement destruction). This is phrased poorly. It doesn't prevent the possibility of a collection. It avoids the mutation of m_windowShells during iteration. I'll rewrite it. Comment on attachment 235386 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=235386&action=review > Source/WebCore/bindings/js/ScriptController.cpp:177 > + return windowShells; will this get happy move semantics? Comment on attachment 235386 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=235386&action=review >> Source/WebCore/bindings/js/ScriptController.cpp:177 >> + return windowShells; > > will this get happy move semantics? will this get happy move semantics? Comment on attachment 235386 [details] [PATCH] Proposed Fix Attachment 235386 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5984569622790144 New failing tests: media/track/add-and-remove-track.html Created attachment 235390 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
(In reply to comment #7) > >> Source/WebCore/bindings/js/ScriptController.cpp:177 > >> + return windowShells; > > > > will this get happy move semantics? I expect it to. I shouldn't need to do anything special, right? (In reply to comment #10) > (In reply to comment #7) > > >> Source/WebCore/bindings/js/ScriptController.cpp:177 > > >> + return windowShells; > > > > > > will this get happy move semantics? > > I expect it to. I shouldn't need to do anything special, right? Asking around, yes this should. I don't need to do anything special. To be explicit, we could WTF::move(...). Comment on attachment 235386 [details] [PATCH] Proposed Fix Clearing flags on attachment: 235386 Committed r171505: <http://trac.webkit.org/changeset/171505> All reviewed patches have been landed. Closing bug. Comment on attachment 235386 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=235386&action=review > Source/WebCore/bindings/js/ScriptCachedFrameData.cpp:54 > + Vector<JSC::Strong<JSDOMWindowShell>> windowShells = scriptController.windowShells(); Can a new window shell be added to m_windowShells during iteration? If this happens, will it remain in a broken state forever, not being subject to the below processing? (In reply to comment #14) > (From update of attachment 235386 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=235386&action=review > > > Source/WebCore/bindings/js/ScriptCachedFrameData.cpp:54 > > + Vector<JSC::Strong<JSDOMWindowShell>> windowShells = scriptController.windowShells(); > > Can a new window shell be added to m_windowShells during iteration? If this happens, will it remain in a broken state forever, not being subject to the below processing? Not currently, otherwise I would have expected to see a crash/assert here before. I also don't think we ever want mutation of a Hash* during iteration, even if it is additive. I believe that in HashMap/HashSet iteration order is not guaranteed; so depending on that behavior would be bad. Please correct me if I'm wrong! We need to prevent against a removal of a window shell in m_windowShells during iteration because that can now happen at any time (GC of a media element). Creating a window shell still happens in only a few places. > I believe that in HashMap/HashSet iteration order is not guaranteed; so depending on that behavior would be bad. Please correct me if I'm wrong! This is certainly correct, one can't iterate over a container that's changing. > We need to prevent against a removal of a window shell in m_windowShells during iteration because that can now happen at any time (GC of a media element). Creating a window shell still happens in only a few places. OK, sounds good. I'm always worried about such copy-to-iterate fixes, because they can be papering over a problem, leaving us with a different random misbehavior that's almost impossible to diagnose. If at all possible, it's desirable to at least to have an ASSERT that we don't have any new elements in the map after iteration. |