Bug 203811

Summary: Collect all documents before iterating in Page::forEachDocument
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cdumez, commit-queue, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Alex Christensen 2019-11-04 07:15:46 PST
Collect all documents before iterating in Page::forEachDocument
Comment 1 Alex Christensen 2019-11-04 07:18:42 PST
Created attachment 382736 [details]
Patch
Comment 2 Alex Christensen 2019-11-04 07:18:47 PST
<rdar://problem/56832747>
Comment 3 Alexey Proskuryakov 2019-11-04 09:44:15 PST
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.
Comment 4 Chris Dumez 2019-11-04 09:45:32 PST
(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 5 Ryosuke Niwa 2019-11-04 13:07:45 PST
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.
Comment 6 Alexey Proskuryakov 2019-11-04 13:24:37 PST
Have other browsers solved this? Should be straightforward to build a test case that's broken with the proposed implementation.
Comment 7 Ryosuke Niwa 2019-11-04 13:26:14 PST
(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.
Comment 8 Ryosuke Niwa 2019-11-04 13:32:59 PST
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 9 WebKit Commit Bot 2019-11-04 13:55:15 PST
Comment on attachment 382736 [details]
Patch

Clearing flags on attachment: 382736

Committed r252013: <https://trac.webkit.org/changeset/252013>
Comment 10 WebKit Commit Bot 2019-11-04 13:55:17 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Alexey Proskuryakov 2019-11-04 16:20:15 PST
"Page::forEachDocument" doesn't say that it only works acceptably well for resize events.
Comment 12 Ryosuke Niwa 2019-11-04 17:27:20 PST
(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.