Detached FrameLoader should not log snapshot trace events

Detached FrameLoader should not log snapshot trace events

FrameLoader emits object lifetime events. The destroy event is logged on
detach(), but after detach(), FrameLoader::takeObjectSnapshot() can be
called from FrameLoader::clear().

We should not log snapshot events after the destroy event, as that
confuses loading metrics (FCP, FMP, TTI).

Note for perf sheriffs: This may impact timeToFirstConentfulPaint,
timeToFirstMeaningfulPaint and timeToFirstInteractive, as this fixes a
bug of these metrics.

BUG=706671
Review-Url: https://codereview.chromium.org/2785873002
Cr-Commit-Position: refs/heads/master@{#460703}
Committed: https://chromium.googlesource.com/chromium/src/+/dd3783e3fbc682b2140333c56f446c1570cbe4b8

[Kunihiko Sakamoto, 2017-03-30 02:18:53]

The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 02:19:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Kunihiko Sakamoto, 2017-03-30 02:48:03]

Description was changed from

==========
Do not take snapshot for detached FrameLoader

BUG=706671
==========

to

==========
Detached FrameLoader should not log snapshot trace events

FrameLoader emits object lifetime events. The destroy event is logged on
detach(), but after detach(), FrameLoader::takeObjectSnapshot() can be
called from FrameLoader::clear().

We should not log snapshot events after the destroy event, as that
confuses loading metrics (FCP, FMP, TTI).

BUG=706671
==========

[Kunihiko Sakamoto, 2017-03-30 02:51:47]

Description was changed from

==========
Detached FrameLoader should not log snapshot trace events

FrameLoader emits object lifetime events. The destroy event is logged on
detach(), but after detach(), FrameLoader::takeObjectSnapshot() can be
called from FrameLoader::clear().

We should not log snapshot events after the destroy event, as that
confuses loading metrics (FCP, FMP, TTI).

BUG=706671
==========

to

==========
Detached FrameLoader should not log snapshot trace events

FrameLoader emits object lifetime events. The destroy event is logged on
detach(), but after detach(), FrameLoader::takeObjectSnapshot() can be
called from FrameLoader::clear().

We should not log snapshot events after the destroy event, as that
confuses loading metrics (FCP, FMP, TTI).

Note for perf sheriffs: This may impact timeToFirstConentfulPaint,
timeToFirstMeaningfulPaint and timeToFirstInteractive, as this fixes a
bug of these metrics.

BUG=706671
==========

[Kunihiko Sakamoto, 2017-03-30 02:57:22]

ksakamoto@chromium.org changed reviewers:
+ kouhei@chromium.org

[commit-bot: I haz the power, 2017-03-30 04:03:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 04:03:13]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2017-03-30 04:49:22]

Relying on m_progressTracker sounds a bit fragile to me

https://codereview.chromium.org/2785873002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2785873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1888: if
(!m_progressTracker) {
Would you m_frame = nullptr in ::detach() and check for m_frame here?

[Kunihiko Sakamoto, 2017-03-30 05:08:58]

https://codereview.chromium.org/2785873002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2785873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1888: if
(!m_progressTracker) {
On 2017/03/30 04:49:22, kouhei wrote:
> Would you m_frame = nullptr in ::detach() and check for m_frame here?

Hmm, ::clear() uses m_frame after ::detach() (called from LocalFrame::detach()),
so we cannot simply clear m_frame in ::detach().

[Kunihiko Sakamoto, 2017-03-30 05:52:22]

PTAL

https://codereview.chromium.org/2785873002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2785873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1888: if
(!m_progressTracker) {
On 2017/03/30 05:08:58, Kunihiko Sakamoto wrote:
> On 2017/03/30 04:49:22, kouhei wrote:
> > Would you m_frame = nullptr in ::detach() and check for m_frame here?
> 
> Hmm, ::clear() uses m_frame after ::detach() (called from
LocalFrame::detach()),
> so we cannot simply clear m_frame in ::detach().

m_frame = nullptr in clear() didn't work either. Here's the clash trace, for the
record:

Received signal 11 SEGV_MAPERR 000000000028
#3 0x7fadfec17f8f blink::FrameLoader::effectiveSandboxFlags()
#4 0x7fadfe6e4652 blink::DocumentInit::getSandboxFlags()
#5 0x7fadfe6c15ab blink::Document::initSecurityContext()
#6 0x7fadfe6c0b07 blink::Document::Document()
#7 0x7fadfe8dad65 blink::HTMLDocument::HTMLDocument()
#8 0x7fadfe6b46c1 blink::HTMLDocument::create()
#9 0x7fadfe6b4872 blink::DOMImplementation::createDocument()
#10 0x7fadfe874db4 blink::LocalDOMWindow::createDocument()
#11 0x7fadfe874f81 blink::LocalDOMWindow::installNewDocument()
#12 0x7fadfec0513b blink::DocumentLoader::installNewDocument()
#13 0x7fadfec05062 blink::DocumentLoader::ensureWriter()
#14 0x7fadfec0448e blink::DocumentLoader::commitData()
#15 0x7fadfec04302 blink::DocumentLoader::finishedLoading()

I gave up clearing m_frame and added |m_detached| flag instead.

[kouhei (in TOK), 2017-03-30 05:56:46]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2017-03-30 05:56:47]

lgtm

[commit-bot: I haz the power, 2017-03-30 05:56:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 08:57:50]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490853406566500,
"parent_rev": "ffa542f8d3c30ecea85ec3ec076facaa42fab8e9", "commit_rev":
"dd3783e3fbc682b2140333c56f446c1570cbe4b8"}

[commit-bot: I haz the power, 2017-03-30 08:58:39]

Description was changed from

==========
Detached FrameLoader should not log snapshot trace events

FrameLoader emits object lifetime events. The destroy event is logged on
detach(), but after detach(), FrameLoader::takeObjectSnapshot() can be
called from FrameLoader::clear().

We should not log snapshot events after the destroy event, as that
confuses loading metrics (FCP, FMP, TTI).

Note for perf sheriffs: This may impact timeToFirstConentfulPaint,
timeToFirstMeaningfulPaint and timeToFirstInteractive, as this fixes a
bug of these metrics.

BUG=706671
==========

to

==========
Detached FrameLoader should not log snapshot trace events

FrameLoader emits object lifetime events. The destroy event is logged on
detach(), but after detach(), FrameLoader::takeObjectSnapshot() can be
called from FrameLoader::clear().

We should not log snapshot events after the destroy event, as that
confuses loading metrics (FCP, FMP, TTI).

Note for perf sheriffs: This may impact timeToFirstConentfulPaint,
timeToFirstMeaningfulPaint and timeToFirstInteractive, as this fixes a
bug of these metrics.

BUG=706671

Review-Url: https://codereview.chromium.org/2785873002
Cr-Commit-Position: refs/heads/master@{#460703}
Committed:
https://chromium.googlesource.com/chromium/src/+/dd3783e3fbc682b2140333c56f44...
==========

[commit-bot: I haz the power, 2017-03-30 08:58:40]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/dd3783e3fbc682b2140333c56f44...