203811 2019-11-04 07:15:46 -0800 Collect all documents before iterating in Page::forEachDocument 2019-11-04 17:27:20 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 achristensen achristensen ap cdumez commit-queue rniwa webkit-bug-importer oldest_to_newest 1586970 0 achristensen 2019-11-04 07:15:46 -0800 Collect all documents before iterating in Page::forEachDocument 1586971 1 382736 achristensen 2019-11-04 07:18:42 -0800 Created attachment 382736 Patch 1586972 2 achristensen 2019-11-04 07:18:47 -0800 <rdar://problem/56832747> 1587028 3 382736 ap 2019-11-04 09:44:15 -0800 Comment on attachment 382736 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. 1587029 4 cdumez 2019-11-04 09:45:32 -0800 (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. 1587109 5 382736 rniwa 2019-11-04 13:07:45 -0800 Comment on attachment 382736 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. 1587120 6 ap 2019-11-04 13:24:37 -0800 Have other browsers solved this? Should be straightforward to build a test case that's broken with the proposed implementation. 1587123 7 rniwa 2019-11-04 13:26:14 -0800 (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. 1587125 8 rniwa 2019-11-04 13:32:59 -0800 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. 1587140 9 382736 commit-queue 2019-11-04 13:55:15 -0800 Comment on attachment 382736 Patch Clearing flags on attachment: 382736 Committed r252013: <https://trac.webkit.org/changeset/252013> 1587141 10 commit-queue 2019-11-04 13:55:17 -0800 All reviewed patches have been landed. Closing bug. 1587223 11 ap 2019-11-04 16:20:15 -0800 "Page::forEachDocument" doesn't say that it only works acceptably well for resize events. 1587234 12 rniwa 2019-11-04 17:27:20 -0800 (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. 382736 2019-11-04 07:18:42 -0800 2019-11-04 13:55:15 -0800 Patch bug-203811-20191104151840.patch text/plain 1641 achristensen SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDI1MTk4NykKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDE5LTExLTA0ICBBbGV4IENo cmlzdGVuc2VuICA8YWNocmlzdGVuc2VuQHdlYmtpdC5vcmc+CisKKyAgICAgICAgQ29sbGVjdCBh bGwgZG9jdW1lbnRzIGJlZm9yZSBpdGVyYXRpbmcgaW4gUGFnZTo6Zm9yRWFjaERvY3VtZW50Cisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMDM4MTEKKyAg ICAgICAgPHJkYXI6Ly9wcm9ibGVtLzU2ODMyNzQ3PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgorCisgICAgICAgIFNvbWUgY2FsbHMgdG8gZm9yRWFjaERvY3VtZW50LCBu b3RhYmx5IHRob3NlIGludHJvZHVjZWQgaW4gcjI1MTkzMCwgY2FuIG11dGF0ZSB0aGUgZnJhbWVz IGFuZCBkb2N1bWVudHMgYXMgdGhleSBhcmUgYmVpbmcgaXRlcmF0ZWQuCisgICAgICAgIFRoaXMg Y2F1c2VzIHByb2JsZW1zIHRoYXQgY2FuIGJlIGF2b2lkZWQgYnkgaXRlcmF0aW5nLCBrZWVwaW5n IGFsbCBEb2N1bWVudHMgcmVmZXJlbmNlZCwgdGhlbiBpdGVyYXRpbmcgdGhlIHJlZmVyZW5jZWQg RG9jdW1lbnRzLgorCisgICAgICAgICogcGFnZS9QYWdlLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6 OlBhZ2U6OmZvckVhY2hEb2N1bWVudCk6CisKIDIwMTktMTEtMDQgIHlvdWVubiBmYWJsZXQgIDx5 b3Vlbm5AYXBwbGUuY29tPgogCiAgICAgICAgIE9yZGVyIG9mIHBvc3RNZXNzYWdlIGFuZCBmZXRj aCBldmVudHMgc2hvdWxkIGJlIHByZXNlcnZlZCB3aGVuIGdvaW5nIGZyb20gY2xpZW50IHRvIHNl cnZpY2Ugd29ya2VyCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wYWdlL1BhZ2UuY3BwCj09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT0KLS0tIFNvdXJjZS9XZWJDb3JlL3BhZ2UvUGFnZS5jcHAJKHJldmlzaW9uIDI1MTk4MCkKKysr IFNvdXJjZS9XZWJDb3JlL3BhZ2UvUGFnZS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTI4NjksMTIg KzI4NjksOCBAQCBSZW5kZXJpbmdVcGRhdGVTY2hlZHVsZXImIFBhZ2U6OnJlbmRlcmluCiAKIHZv aWQgUGFnZTo6Zm9yRWFjaERvY3VtZW50KGNvbnN0IEZ1bmN0aW9uPHZvaWQoRG9jdW1lbnQmKT4m IGZ1bmN0b3IpCiB7Ci0gICAgZm9yIChGcmFtZSogZnJhbWUgPSAmbWFpbkZyYW1lKCk7IGZyYW1l OyBmcmFtZSA9IGZyYW1lLT50cmVlKCkudHJhdmVyc2VOZXh0KCkpIHsKLSAgICAgICAgaWYgKCFm cmFtZS0+ZG9jdW1lbnQoKSkKLSAgICAgICAgICAgIGNvbnRpbnVlOwotCi0gICAgICAgIGZ1bmN0 b3IoKmZyYW1lLT5kb2N1bWVudCgpKTsKLSAgICB9CisgICAgZm9yIChhdXRvJiBkb2N1bWVudCA6 IGNvbGxlY3REb2N1bWVudHMoKSkKKyAgICAgICAgZnVuY3Rvcihkb2N1bWVudCk7CiB9CiAKIFZl Y3RvcjxSZWY8RG9jdW1lbnQ+PiBQYWdlOjpjb2xsZWN0RG9jdW1lbnRzKCkK