Gracefully handle a self-referential opener in frame swap.

Gracefully handle a self-referential opener in frame swap.

BUG=543949
Committed: https://crrev.com/f498eb4d024e1af5efcce40c5f6116684febf55a
Cr-Commit-Position: refs/heads/master@{#356443}

[dcheng, 2015-10-27 01:00:26]

Description was changed from

==========
Avoid nulling out WebFrame::m_openedFrameTracker before destructor.

BUG=543949
==========

to

==========
Gracefully handle a self-referential opener in frame swap.

BUG=543949
==========

[dcheng, 2015-10-27 01:00:26]

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

[dcheng, 2015-10-27 01:33:12]

initializeToReplaceRemoteFrame gives the provisional local frame the same opener
as the remote frame it might replace.

Then inside WebFrame::swap():
  swap(m_opener, frame->m_opener);

ends up being a no-op: both frames have a non-null opener.

Shortly afterwards, we transfer OpenedFrameTracker:
  frame->m_openedFrameTracker.reset(m_openedFrameTracker.release());

So now this->m_openedFrameTracker is null, but m_opener == this.

Later on, in this->detach(), we crash. Since the opener is not null, we clear
it. When we clear it, it tries to remove this from the original opener's list of
tracked windows. But in this case, the original opener == this and
this->m_openedFrameTracker is null... explosions.

I think I like the new code better anyway. Less friend declarations, at the cost
of a minor bit of efficiency (by copying the HashSet). But less messing around
in the state of other classes hopefully means that this code will be more
resistant to explosions.

[alexmos, 2015-10-27 19:09:51]

LGTM.  This was tricky, thanks for fixing!  I added that setOpener call in
initializeToReplaceRemoteFrame so that the provisional frame's document inherits
the opener's origin during init (https://crbug.com/515571), but didn't realize
it had this effect on WebFrame::swap.

https://codereview.chromium.org/1408073006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/OpenedFrameTracker.cpp (right):

https://codereview.chromium.org/1408073006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/OpenedFrameTracker.cpp:42: HashSet<WebFrame*>
frames(m_openedFrames);
nit: is it worth adding a comment about why the copy is necessary here? (because
setOpener modifies m_openedFrames, right?)

[dcheng, 2015-10-27 19:11:16]

https://codereview.chromium.org/1408073006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/OpenedFrameTracker.cpp (right):

https://codereview.chromium.org/1408073006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/OpenedFrameTracker.cpp:42: HashSet<WebFrame*>
frames(m_openedFrames);
On 2015/10/27 at 19:09:51, alexmos wrote:
> nit: is it worth adding a comment about why the copy is necessary here?
(because setOpener modifies m_openedFrames, right?)

Yeah, probably worth commenting. I'll add it and CQ it after lunch.

[dcheng, 2015-10-27 20:28:08]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-10-27 20:28:08]

The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/1408073006/#ps60001
(title: "Add comment")

[commit-bot: I haz the power, 2015-10-27 20:29:15]

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

[commit-bot: I haz the power, 2015-10-27 23:57:01]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-27 23:58:06]

Patchset 4 (id:??) landed as
https://crrev.com/f498eb4d024e1af5efcce40c5f6116684febf55a
Cr-Commit-Position: refs/heads/master@{#356443}