Bug 107302 – Assert the connectedSubframeCount is consistent and fix over counting

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

[Elliott Sprehn]

Assert the connectedSubframeCount is consistent in debug

[Elliott Sprehn]

Created attachment 183501 [details]
Patch

[Elliott Sprehn]

Created attachment 183502 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 183502 [details]
Patch

r=me, although I don't see a reason to not check consistency every time connectedSubframeCount() is called. That would catch errors much closer to when they occur.

[Elliott Sprehn]

(In reply to comment #3)
> (From update of attachment 183502 [details])
> r=me, although I don't see a reason to not check consistency every time connectedSubframeCount() is called. That would catch errors much closer to when they occur.

Because we call that as we walk down the tree in collectFrameOwners() and during tree building, which would mean we'd be walking the subtree, over and over again at every level of the tree making removeChild and parser tree building n^2 in debug builds.

[Alexey Proskuryakov]

Sure, but weren't we doing the same before bug 101821, without any practical consequences?

[Elliott Sprehn]

(In reply to comment #5)
> Sure, but weren't we doing the same before bug 101821, without any practical consequences?

We only walked the subtree once, it wasn't n^2, and never during tree building in parsing.

[Build Bot]

Comment on attachment 183502 [details]
Patch

Attachment 183502 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15948532

[Elliott Sprehn]

This uncovered a case where we were counting incorrectly due to the way frame tree destruction happens, the incorrect counting is that we over count though, so there's no security problem, we'll just walk the tree looking for loaded frames that are no longer loaded.

I'll add a test and reupload.

[Elliott Sprehn]

(In reply to comment #8)
> This uncovered a case where we were counting incorrectly due to the way frame tree destruction happens, the incorrect counting is that we over count though, so there's no security problem, we'll just walk the tree looking for loaded frames that are no longer loaded.
> 
> I'll add a test and reupload.

Also note that fixing this means we remove a full document traversal during document destruction, so navigating child frames or unloading pages will be faster.

ex. window.location = '...'; or frame.contentWindow.location = '...';

[Elliott Sprehn]

Created attachment 184085 [details]
Patch

[Elliott Sprehn]

(In reply to comment #10)
> Created an attachment (id=184085) [details]
> Patch

This is ready for review again. I found no cases of undercounting (security badness) but two cases of overcounting which this fixes.

[Elliott Sprehn]

Created attachment 184335 [details]
Patch

[Elliott Sprehn]

Created attachment 184336 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 184336 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184336&action=review

I didn't review particularly deeply, but looks fine. It's a great feeling when adding a check uncovers an actual bug.

> Source/WebCore/dom/ContainerNodeAlgorithms.cpp:125
> +unsigned assertConnectedSubframeCountIsConsistent(Node* node)

I don't get what it means when an "assert" function returns a value. I don't think that it should be named like this in this case.

> Source/WebCore/dom/ContainerNodeAlgorithms.cpp:145
> +    // If we overcount it's safe, but not optimal. 

It would be helpful to describe in more detail here what is non-optimal. Will we leak? Or will we need to traverse frame tree an extra time somewhere?

[Elliott Sprehn]

Comment on attachment 184336 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184336&action=review

>> Source/WebCore/dom/ContainerNodeAlgorithms.cpp:125
>> +unsigned assertConnectedSubframeCountIsConsistent(Node* node)
> 
> I don't get what it means when an "assert" function returns a value. I don't think that it should be named like this in this case.

I don't know what else to call it. It recursively asserts as it goes down the tree, so it needs the return value. In a release build all it would do is computeConnectedSubframeCount(), but I don't want anyone to use it outside of debug and calling computeConnectedSubframeCount() inside disconnect() and discarding the value seems pretty weird. Have an idea?

>> Source/WebCore/dom/ContainerNodeAlgorithms.cpp:145
>> +    // If we overcount it's safe, but not optimal. 
> 
> It would be helpful to describe in more detail here what is non-optimal. Will we leak? Or will we need to traverse frame tree an extra time somewhere?

Done.

[Elliott Sprehn]

Created attachment 184707 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 184707 [details]
Patch for landing

Clearing flags on attachment: 184707

Committed r140807: <http://trac.webkit.org/changeset/140807>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

I think that "recursiveCheck..." would be better. Asserts never return values, a "check" function has more leeway.

[Elliott Sprehn]

(In reply to comment #19)
> I think that "recursiveCheck..." would be better. Asserts never return values, a "check" function has more leeway.

Okay, I'll rename in a followup patch.