Fix the logic that limits the number of frames in a page.

Fix the logic that limits the number of frames in a page.

This check apparently doesn't run soon enough, and we can create more than the
intended limit of 1000 frames. Once we hit 1024,
NodeRareData::m_connecetedFrameCount can overflow and we no longer fully detach
Frames from their owners at teardown.

BUG=493243
TEST=WebFrameTest.MaxFramesDetach

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197139

[Nate Chapin, 2015-06-10 22:02:02]

japhet@chromium.org changed reviewers:
+ dcheng@chromium.org

[Nate Chapin, 2015-06-10 22:02:03]

The test case in the change description averaged 9 seconds per run on my Z620 in
a release build, so adding a proper regression test seems terrible.

[dcheng, 2015-06-10 22:08:05]

https://codereview.chromium.org/1180603002/diff/1/Source/core/dom/NodeRareData.h
File Source/core/dom/NodeRareData.h (right):

https://codereview.chromium.org/1180603002/diff/1/Source/core/dom/NodeRareDat...
Source/core/dom/NodeRareData.h:87:
RELEASE_ASSERT_WITH_SECURITY_IMPLICATION((m_connectedFrameCount + amount) < (1
<< ConnectedFrameCountBits));
Would it make sense to use maxNumberOfFrames here too?

[dcheng, 2015-06-10 22:08:37]

Also... just curious, but would a unit test run any more quickly, since we don't
have the full content stack?

[Nate Chapin, 2015-06-10 22:13:47]

https://codereview.chromium.org/1180603002/diff/1/Source/core/dom/NodeRareData.h
File Source/core/dom/NodeRareData.h (right):

https://codereview.chromium.org/1180603002/diff/1/Source/core/dom/NodeRareDat...
Source/core/dom/NodeRareData.h:87:
RELEASE_ASSERT_WITH_SECURITY_IMPLICATION((m_connectedFrameCount + amount) < (1
<< ConnectedFrameCountBits));
On 2015/06/10 22:08:05, dcheng wrote:
> Would it make sense to use maxNumberOfFrames here too?

I don't feel strongly, though this impl would need to move to the .cpp, so we
don't include FrameHost.h here unnecessarily.

[Nate Chapin, 2015-06-10 23:04:23]

On 2015/06/10 22:08:37, dcheng wrote:
> Also... just curious, but would a unit test run any more quickly, since we
don't
> have the full content stack?

That makes a much bigger difference than I would have expected (500ms vs 9sec,
though it still takes 3sec in debug for me). As written, it doesn't UAF like the
layout test did, but it reliably asserts in debug without the fix.

[dcheng, 2015-06-12 20:38:37]

LGTM.

I'm curious if calling WebLocalFrame::collectGarbage() will trigger the ASAN
error. If it does, it might be nice to add that line into the test. Otherwise,
this is fine too. Sorry for the delay on the reply.

[Nate Chapin, 2015-06-15 19:56:31]

On 2015/06/12 20:38:37, dcheng wrote:
> LGTM.
> 
> I'm curious if calling WebLocalFrame::collectGarbage() will trigger the ASAN
> error. If it does, it might be nice to add that line into the test. Otherwise,
> this is fine too. Sorry for the delay on the reply.

Huh. I figured you'd have to do something after GC to get the UAF, but
apparently collectGarbage() is sufficient. Updated.

[Nate Chapin, 2015-06-15 19:56:40]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2015-06-15 19:56:40]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1180603002/#ps60001
(title: "+UAF")

[commit-bot: I haz the power, 2015-06-15 19:57:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180603002/60001

[commit-bot: I haz the power, 2015-06-15 21:26:48]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197139