Collect all documents before iterating in Page::forEachDocument
Created attachment 382736 [details] Patch
<rdar://problem/56832747>
Comment on attachment 382736 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382736&action=review > Source/WebCore/page/Page.cpp:2873 > + for (auto& document : collectDocuments()) > + functor(document); While this pattern prevents some problems, it's inherently incorrect too. Callers of functions like "forEachDocument" would naturally expect that all documents had the functor applied after the call, which is not going to happen in case of additions.
(In reply to Alexey Proskuryakov from comment #3) > Comment on attachment 382736 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382736&action=review > > > Source/WebCore/page/Page.cpp:2873 > > + for (auto& document : collectDocuments()) > > + functor(document); > > While this pattern prevents some problems, it's inherently incorrect too. > Callers of functions like "forEachDocument" would naturally expect that all > documents had the functor applied after the call, which is not going to > happen in case of additions. I agree with Alexey. Seems like the issue may be with the call sites which modify the frame tree as they iterate.
Comment on attachment 382736 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382736&action=review >>> Source/WebCore/page/Page.cpp:2873 >>> + functor(document); >> >> While this pattern prevents some problems, it's inherently incorrect too. Callers of functions like "forEachDocument" would naturally expect that all documents had the functor applied after the call, which is not going to happen in case of additions. > > I agree with Alexey. Seems like the issue may be with the call sites which modify the frame tree as they iterate. If we're firing resize event, then it's inherently possible for those event listeners to modify the frame tree structure. I don't think we can simultaneously satisfy the need to fire scripts and iterating all frames at the same time.
Have other browsers solved this? Should be straightforward to build a test case that's broken with the proposed implementation.
(In reply to Alexey Proskuryakov from comment #6) > Have other browsers solved this? Should be straightforward to build a test > case that's broken with the proposed implementation. If a new frame is inserted in the midst of resize event, that document hasn't been "resized" because it just got inserted. Similar for other viewport related events.
By the way, our implementation of all these events are so off the standard that the last thing I'm worried about will be the edge case of an iframe inserted during other resize events. We'd need to first move the time we compute whether a resize event is needed or not to Page::updateRendering. Unfortunately, I don't think I'll ever get to it in time.
Comment on attachment 382736 [details] Patch Clearing flags on attachment: 382736 Committed r252013: <https://trac.webkit.org/changeset/252013>
All reviewed patches have been landed. Closing bug.
"Page::forEachDocument" doesn't say that it only works acceptably well for resize events.
(In reply to Alexey Proskuryakov from comment #11) > "Page::forEachDocument" doesn't say that it only works acceptably well for > resize events. I mean, we do this elsewhere in WebCore for Node, etc... none of those code explicitly say things may fail or skip even though they would. I don’t think this iterator function is any different from that.