Bug 203811 – Collect all documents before iterating in Page::forEachDocument

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Collect all documents before iterating in Page::forEachDocument

[Alex Christensen]

Created attachment 382736 [details]
Patch

[Alex Christensen]

<rdar://problem/56832747>

[Alexey Proskuryakov]

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.

[Chris Dumez]

(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.

[Ryosuke Niwa]

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.

[Alexey Proskuryakov]

Have other browsers solved this? Should be straightforward to build a test case that's broken with the proposed implementation.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

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.

[WebKit Commit Bot]

Comment on attachment 382736 [details]
Patch

Clearing flags on attachment: 382736

Committed r252013: <https://trac.webkit.org/changeset/252013>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

"Page::forEachDocument" doesn't say that it only works acceptably well for resize events.

[Ryosuke Niwa]

(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.