Summary: | Assert the connectedSubframeCount is consistent and fix over counting | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Elliott Sprehn <esprehn> | ||||||||||||||
Component: | New Bugs | Assignee: | Elliott Sprehn <esprehn> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | ap, ojan.autocc, ojan, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 101821 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Elliott Sprehn
2013-01-18 10:31:21 PST
Created attachment 183501 [details]
Patch
Created attachment 183502 [details]
Patch
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.
(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. Sure, but weren't we doing the same before bug 101821, without any practical consequences? (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. Comment on attachment 183502 [details] Patch Attachment 183502 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15948532 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. (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 = '...'; Created attachment 184085 [details]
Patch
(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. Created attachment 184335 [details]
Patch
Created attachment 184336 [details]
Patch
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? 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. Created attachment 184707 [details]
Patch for landing
Comment on attachment 184707 [details] Patch for landing Clearing flags on attachment: 184707 Committed r140807: <http://trac.webkit.org/changeset/140807> All reviewed patches have been landed. Closing bug. I think that "recursiveCheck..." would be better. Asserts never return values, a "check" function has more leeway. (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. |