TextAutosizer should handle cross-process navigation and RemoteFrame.

TextAutosizer should handle cross-process navigation and RemoteFrame.

In https://codereview.chromium.org/1374783005/ the fix tested for whether
the frame has a FrameView or not. The problem turns out to be a bit
different - the check for RemoteFrame and the mainFrame do not point
to the same object, which leaves the code still open to problems.

This CL is ensuring that both the check for RemoteFrame and the usage of
the main LocalFrame point to the same object.

BUG=357747
Committed: https://crrev.com/885b6b65af2d7504cdda6091516d24704d2400b3
Cr-Commit-Position: refs/heads/master@{#352249}

[nasko, 2015-10-02 20:48:58]

nasko@chromium.org changed reviewers:
+ skobes@chromium.org

[nasko, 2015-10-02 20:48:59]

Hi Steve,
Can you review this CL for me? My previous one wasn't the best approach, as I
missed that we are checking two potentially different objects. It wasn't
immediately obvious and I think the code is more straightforward this way.
Thanks in advance!
Nasko

[skobes, 2015-10-03 01:32:17]

lgtm

Code change looks fine but I don't understand the description.  Are you saying
Page::m_mainFrame may not be at the top of the FrameTree?

[nasko, 2015-10-03 06:52:33]

On 2015/10/03 01:32:17, skobes wrote:
> lgtm
> 
> Code change looks fine but I don't understand the description.  Are you saying
> Page::m_mainFrame may not be at the top of the FrameTree?

Page::m_mainFrame does point to the top of the FrameTree, but it has a
RemoteFrame and is the only frame in the case of a page without iframes. When
transitioning from a RemoteFrame to LocalFrame, we create the LocalFrame, but
don't make it part of Page until it commits. It is at that time that
Page::m_mainFrame will start pointing to the LocalFrame.

So my original fix was checking the document->page()->mainFrame(), which is
RemoteFrame and when we need to get a LocalFrame out of it, it crashes. Instead,
we should use the same frame we've checked for being remote or not few lines
above.

All of this will be much simpler if the TextAutosizer wasn't trying to compute
anything until we actually have a real document in place. The crashing stack is
as follows:

013f2048  blink::toLocalFrame(blink::Frame*)+44
01b9b451  blink::TextAutosizer::updatePageInfo()+226
016e0f9d  blink::Document::attach(blink::Node::AttachContext const&)+184
019f8af5  blink::LocalDOMWindow::installNewDocument(WTF::String const&,
blink::DocumentInit const&, bool)+172
01a76e33  blink::DocumentLoader::createWriterFor(blink::Document const*,
blink::DocumentInit const&, WTF::AtomicString const&, WTF::AtomicString const&,
bool, blink::ParserSynchronizationPolicy)+190
01a77093  blink::DocumentLoader::ensureWriter(WTF::AtomicString const&,
blink::KURL const&)+202
01a7766b  blink::DocumentLoader::commitData(char const*, unsigned int)+82
01a77adb  blink::DocumentLoader::finishedLoading(double)+158
01a7889b  blink::DocumentLoader::maybeLoadEmpty()+762
01a7899f  blink::DocumentLoader::startLoadingMainResource()+222
01a810ff  blink::FrameLoader::init()+152
0142902d 
blink::WebLocalFrameImpl::initializeToReplaceRemoteFrame(blink::WebRemoteFrame*,
blink::WebString const&, blink::WebSandboxFlags)+208

Since this is the initial empty document, we haven't inserted the frame into the
frame tree quite yet, but we will in a short period of time, as we wait for the
real document commit.

I hope this helps explain it better.

[nasko, 2015-10-03 06:52:37]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2015-10-03 06:52:51]

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

[commit-bot: I haz the power, 2015-10-03 06:57:38]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-10-03 06:58:29]

Patchset 1 (id:??) landed as
https://crrev.com/885b6b65af2d7504cdda6091516d24704d2400b3
Cr-Commit-Position: refs/heads/master@{#352249}

[skobes, 2015-10-05 01:25:47]

On 2015/10/03 06:52:33, nasko (slow to review) wrote:
> On 2015/10/03 01:32:17, skobes wrote:
> > lgtm
> > 
> > Code change looks fine but I don't understand the description.  Are you
saying
> > Page::m_mainFrame may not be at the top of the FrameTree?
> 
> Page::m_mainFrame does point to the top of the FrameTree, but it has a
> RemoteFrame and is the only frame in the case of a page without iframes. When
> transitioning from a RemoteFrame to LocalFrame, we create the LocalFrame, but
> don't make it part of Page until it commits. It is at that time that
> Page::m_mainFrame will start pointing to the LocalFrame.
> 
> So my original fix was checking the document->page()->mainFrame(), which is
> RemoteFrame and when we need to get a LocalFrame out of it, it crashes.
Instead,
> we should use the same frame we've checked for being remote or not few lines
> above.
> 
> All of this will be much simpler if the TextAutosizer wasn't trying to compute
> anything until we actually have a real document in place. The crashing stack
is
> as follows:
> 
> 013f2048  blink::toLocalFrame(blink::Frame*)+44
> 01b9b451  blink::TextAutosizer::updatePageInfo()+226
> 016e0f9d  blink::Document::attach(blink::Node::AttachContext const&)+184
> 019f8af5  blink::LocalDOMWindow::installNewDocument(WTF::String const&,
> blink::DocumentInit const&, bool)+172
> 01a76e33  blink::DocumentLoader::createWriterFor(blink::Document const*,
> blink::DocumentInit const&, WTF::AtomicString const&, WTF::AtomicString
const&,
> bool, blink::ParserSynchronizationPolicy)+190
> 01a77093  blink::DocumentLoader::ensureWriter(WTF::AtomicString const&,
> blink::KURL const&)+202
> 01a7766b  blink::DocumentLoader::commitData(char const*, unsigned int)+82
> 01a77adb  blink::DocumentLoader::finishedLoading(double)+158
> 01a7889b  blink::DocumentLoader::maybeLoadEmpty()+762
> 01a7899f  blink::DocumentLoader::startLoadingMainResource()+222
> 01a810ff  blink::FrameLoader::init()+152
> 0142902d 
>
blink::WebLocalFrameImpl::initializeToReplaceRemoteFrame(blink::WebRemoteFrame*,
> blink::WebString const&, blink::WebSandboxFlags)+208
> 
> Since this is the initial empty document, we haven't inserted the frame into
the
> frame tree quite yet, but we will in a short period of time, as we wait for
the
> real document commit.
> 
> I hope this helps explain it better.

Yes that helps, thanks!