PaintTiming metrics for child frames are sometimes associated with the parent frame's document |
|||||||||
Issue description
I'm working on some code to track paint events in child frames in page_load_metrics, to help us support tracking of metrics for AMP pages.
In debugging this, I discovered that it seems there's one PaintTiming instance for the root document, and all paint updates from child frames are routed to that root instance.
We dispatch that paints occurred in FrameView::paintTree().
This method has the following assertion:
ASSERT(frame() == page()->mainFrame() ||
(!frame().tree().parent()->isLocalFrame()));
In other words, we only run paintTree() from the main frame (unless we're in OOPIF - more on that below).
Only recording metrics associated with the top document is mostly desirable behavior for page_load_metrics (not sure though how this interacts with our FMP calculator - does it assume it's only receiving paint events for a single document?).
However, I think this will be problematic for exposing paint metrics via web perf APIs. My understanding is that in order to avoid leaking cross origin information, we need to scope all such metrics exposed to the web platform to the document they occur in. This likely means associating paint events in child iframes with the document of that frame, for the purpose of metrics tracking.
If we exposed a web perf firstPaint() API in the top document, and that could leak the time of the FP in a child doc, it would open up the framed doc to paint timing attacks. Imagine for example that FP happens differently depending on whether a user is logged in to a page or not. Such an API could be used to infer whether a user is logged in to an iframed page.
RE: OOPIF, the paint tracking behavior is a bit different - in OOPIF it appears that we do paint once per sandboxed iframe, and I believe our metrics do get associated with the correct doc/frame in this case, though I haven't debugged thoroughly yet.
,
Mar 27 2017
Thanks for tracking this down Bryan. Are the (first*) paint updates from child frames getting dropped and never reported to UMA & tracing? I'll take a closer look later today.
,
Mar 27 2017
Thanks! With OOPIF enabled, they do get dropped. I've started to work on a change to add support for reporting timing metrics from child frames in page load metrics, which will address this shortcoming soon. Without OOPIF enabled, paint updates from child frames get reported to the main frame's document. This is arguably the preferred behavior for page load metrics, though it'd be better if we were able to tell which frame a given paint event occurred in, but is not the right behavior for web platform APIs which should be scoped to a single document (or at least a single origin).
,
Mar 27 2017
BTW if it is helpful I can add a page load metrics browsertest that demonstrates the current behavior. It'll have to be disabled for now since it won't pass, but that'll at least provide a starting point to work from.
,
Mar 27 2017
(without oopif) If child frame paint events get reported to the main document, does it track child paint events separately from toplevel? (I don't see this anywhere, code pointers?) Otherwise doesn't that confuse the reporting in UMA / tracing?
,
Apr 5 2017
chatted offline -- I understand now that this happens to be desirable behavior for UMA. i.e. if child iframe's paint fires before parent -- then that becomes the parent's first paint. Obviously -- this won't work for Web Perf APIs. We'll need to update the existing instrumentation to track paint for all the children. Then pick the earliest one for UMA, report pertinent one for Web Perf API etc.
,
Apr 5 2017
We made a layout test (not submittable yet) to demonstrate the issue: https://codereview.chromium.org/2799743003/ Note that the FP, FCP in child iframe is never delivered and printfs indicate that it's going to the parent PaintTiming.
,
Apr 6 2017
+shivanisha FYI, this may impact subresource filtering getting notified of per-frame FCP.
,
Apr 6 2017
Do we use this logic for FMP? i.e., a child's FMP is considered FMP for the parent?
,
Apr 6 2017
For FMP it's a good question. From a user standpoint in the UMA domain where we want to understand the whole page experience, ideally we'd take all frame activity into account (not just the main frame), but I'm not sure how feasible that is for FMP. For FCP I think we should definitely take the earliest FCP in all frames for UMA.
,
Apr 6 2017
The correct behavior for FMP isn't super clear, but saying the main frame has hit FMP because a child frame has hit FMP would definitely be wrong.
,
Apr 6 2017
Agree that we can't take the min FMP across all frames - we'd need to somehow understand meaningful paint on the page as a whole, looking at all frames, which may be very difficult especially in an OOPIF land. So this bug may be less relevant for FMP. But first paint, first contentful paint, first text paint, and first image paint are affected.
,
Apr 7 2017
In the current implementation, FMP isn't affected by this bug because FirstMeaningfulPaintDetector only counts layout objects created within the associated frame.
,
Apr 25 2017
A few questions: * does this bug block our ability to launch first paint API? * what are the next steps for this bug?
,
Apr 30 2017
,
May 1 2017
Zhen is looking at this and will recommend next steps. And yes this is a blocker for shipping FP / FCP APIs
,
May 1 2017
,
May 2 2017
I think this comes down to what developers are expecting this API to do. Currently, what FCP means is not very clear when iframes are present: 1. FCP can be scoped to the whole page covering all frames. 2. FCP can be scoped to individual iframes. 3. FCP can be scoped to individual iframe, and also all its child frames. Given the code pointer from #1, current implementation is that only main frame FCP is reported. Subframe FCPs are not reported. This is also consistent with the layout test result in #7. Bryan, just want to clarify, currently the data reported to UMA via PageLoadMetrics is getting FCPs from main frames only. And that does not take child frames into account, because child frames are not reporting FCPs anyway. Is that what UMA expecting?
,
May 2 2017
The current impl gets the first paint across all frames, unless OOPIF is enabled, in which case it's limited to frames that are in the main frame's process. I started working on reporting timing for child frames as well (it's an easy change) but blocked that work on this bug. For UMA, I think first paint across all frames is probably the right thing. For web platform APIs, first paint needs to be scoped to the document level, to avoid cross origin timing leaks.
,
May 2 2017
I am confused about: "The current impl gets the first paint across all frames, unless OOPIF is enabled, in which case it's limited to frames that are in the main frame's process." As you mentioned in #1, the ASSERT means we only run paintTree() from the main frame, in which FrameView::NotifyPaint is called. How does subframes report FCP to the main frame?
,
May 2 2017
My understanding and observation is that paintTree traverses through all child frames under the main frame. I think Shubhie also confirmed this behavior.
,
May 2 2017
That is, paintTree traverses through all child frames under the main frame that are in the same process as the main frame.
,
May 3 2017
If that's the case, I'm not sure why the layout test shows that the paint in the child iframe never delivers anything to the parent / main frame?
,
May 3 2017
Ah, maybe I misunderstood - I see your earlier comment: "Note that the FP, FCP in child iframe is never delivered and printfs indicate that it's going to the parent PaintTiming." - were you seeing the child frame's timings being reported in the parent frame?
,
May 3 2017
Nope the child frame's timings are NOT reported in the parent frame. The test times out waiting for that.
,
May 3 2017
Ok, - I'm probably misunderstanding something here, but in the linked change: https://codereview.chromium.org/2799743003/ it doesn't look like the parent frame is waiting for anything - it looks to me like all waiting is happening in the iframe'd child "performance-paint-timing-observable.html". Where is the parent document waiting for timing data from the child frame in this test?
,
May 3 2017
Sorry about that, I made a demo and it does validate what you said: the child's paint is getting delivered to the parent. https://panicker.users.x20web.corp.google.com/www/paint/testiframe.html
,
May 15 2017
I just landed https://codereview.chromium.org/2859393002, which adds support in page load metrics for merging timing information across frames to compute first paint / first contentful paint / etc for the whole page. As part of that change, I added some logic in chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc to work around this bug. That test file will need to be updated as part of fixing this bug. You can find the parts that need updating by searching for this bug's id in that file. Ping me if you have any questions, and please CC me on the fix. Thanks!
,
May 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5c0563522bdf7cb4c56420cd7d39c0139b5f8f1a commit 5c0563522bdf7cb4c56420cd7d39c0139b5f8f1a Author: zhenw <zhenw@chromium.org> Date: Mon May 15 22:19:49 2017 Notify paint for each frame Currently main frame reports paint timing and subframes report paint timing via main frame. The desired behavior is that each frame reports its own paint timing. BUG= 705315 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2872793002 Cr-Commit-Position: refs/heads/master@{#471922} [modify] https://crrev.com/5c0563522bdf7cb4c56420cd7d39c0139b5f8f1a/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc [modify] https://crrev.com/5c0563522bdf7cb4c56420cd7d39c0139b5f8f1a/chrome/test/data/page_load_metrics/empty_iframe.html [modify] https://crrev.com/5c0563522bdf7cb4c56420cd7d39c0139b5f8f1a/chrome/test/data/page_load_metrics/iframe.html [modify] https://crrev.com/5c0563522bdf7cb4c56420cd7d39c0139b5f8f1a/chrome/test/data/page_load_metrics/iframes.html [modify] https://crrev.com/5c0563522bdf7cb4c56420cd7d39c0139b5f8f1a/chrome/test/data/page_load_metrics/main_frame_with_iframe.html [modify] https://crrev.com/5c0563522bdf7cb4c56420cd7d39c0139b5f8f1a/third_party/WebKit/Source/core/frame/FrameView.cpp [modify] https://crrev.com/5c0563522bdf7cb4c56420cd7d39c0139b5f8f1a/third_party/WebKit/Source/core/frame/FrameView.h [modify] https://crrev.com/5c0563522bdf7cb4c56420cd7d39c0139b5f8f1a/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp [modify] https://crrev.com/5c0563522bdf7cb4c56420cd7d39c0139b5f8f1a/third_party/WebKit/Source/core/paint/BUILD.gn [add] https://crrev.com/5c0563522bdf7cb4c56420cd7d39c0139b5f8f1a/third_party/WebKit/Source/core/paint/FramePaintTiming.h [modify] https://crrev.com/5c0563522bdf7cb4c56420cd7d39c0139b5f8f1a/third_party/WebKit/Source/core/paint/FramePainter.cpp [modify] https://crrev.com/5c0563522bdf7cb4c56420cd7d39c0139b5f8f1a/third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp [modify] https://crrev.com/5c0563522bdf7cb4c56420cd7d39c0139b5f8f1a/third_party/WebKit/Source/platform/graphics/paint/PaintController.h [modify] https://crrev.com/5c0563522bdf7cb4c56420cd7d39c0139b5f8f1a/third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp
,
May 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/134190a648fe48fed0486cf8889569863e4a544c commit 134190a648fe48fed0486cf8889569863e4a544c Author: kolos <kolos@chromium.org> Date: Tue May 16 12:39:45 2017 Revert of Notify paint for each frame (patchset #12 id:220001 of https://codereview.chromium.org/2872793002/ ) Reason for revert: This CL might be a culprit of test flakiness. https://bugs.chromium.org/p/chromium/issues/detail?id=722703#c2 Original issue's description: > Notify paint for each frame > > Currently main frame reports paint timing and subframes report paint > timing via main frame. The desired behavior is that each frame reports > its own paint timing. > > BUG= 705315 > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 > > Review-Url: https://codereview.chromium.org/2872793002 > Cr-Commit-Position: refs/heads/master@{#471922} > Committed: https://chromium.googlesource.com/chromium/src/+/5c0563522bdf7cb4c56420cd7d39c0139b5f8f1a TBR=chrishtr@chromium.org,bmcquade@chromium.org,panicker@chromium.org,wangxianzhu@chromium.org,zhenw@google.com,zhenw@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 705315 Review-Url: https://codereview.chromium.org/2885773002 Cr-Commit-Position: refs/heads/master@{#472068} [modify] https://crrev.com/134190a648fe48fed0486cf8889569863e4a544c/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc [modify] https://crrev.com/134190a648fe48fed0486cf8889569863e4a544c/chrome/test/data/page_load_metrics/empty_iframe.html [modify] https://crrev.com/134190a648fe48fed0486cf8889569863e4a544c/chrome/test/data/page_load_metrics/iframe.html [modify] https://crrev.com/134190a648fe48fed0486cf8889569863e4a544c/chrome/test/data/page_load_metrics/iframes.html [modify] https://crrev.com/134190a648fe48fed0486cf8889569863e4a544c/chrome/test/data/page_load_metrics/main_frame_with_iframe.html [modify] https://crrev.com/134190a648fe48fed0486cf8889569863e4a544c/third_party/WebKit/Source/core/frame/FrameView.cpp [modify] https://crrev.com/134190a648fe48fed0486cf8889569863e4a544c/third_party/WebKit/Source/core/frame/FrameView.h [modify] https://crrev.com/134190a648fe48fed0486cf8889569863e4a544c/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp [modify] https://crrev.com/134190a648fe48fed0486cf8889569863e4a544c/third_party/WebKit/Source/core/paint/BUILD.gn [delete] https://crrev.com/c1ccb162ddf3f95512c79c2d500d1acb6f146112/third_party/WebKit/Source/core/paint/FramePaintTiming.h [modify] https://crrev.com/134190a648fe48fed0486cf8889569863e4a544c/third_party/WebKit/Source/core/paint/FramePainter.cpp [modify] https://crrev.com/134190a648fe48fed0486cf8889569863e4a544c/third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp [modify] https://crrev.com/134190a648fe48fed0486cf8889569863e4a544c/third_party/WebKit/Source/platform/graphics/paint/PaintController.h [modify] https://crrev.com/134190a648fe48fed0486cf8889569863e4a544c/third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp
,
May 16 2017
,
May 16 2017
,
May 16 2017
,
May 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2701c48359353889f90c30309ff523273890cca7 commit 2701c48359353889f90c30309ff523273890cca7 Author: zhenw <zhenw@chromium.org> Date: Wed May 17 15:17:32 2017 Reland - Notify paint for each frame This CL relands https://crrev.com/2872793002/ after Bryan fixes https://crbug.com/722860 . Currently main frame reports paint timing and subframes report paint timing via main frame. The desired behavior is that each frame reports its own paint timing. BUG= 705315 TBR=bmcquade@chromium.org,chrishtr@chromium.org,wangxianzhu@chromium.org,panicker@chromium.org, CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2884363003 Cr-Commit-Position: refs/heads/master@{#472457} [modify] https://crrev.com/2701c48359353889f90c30309ff523273890cca7/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc [modify] https://crrev.com/2701c48359353889f90c30309ff523273890cca7/chrome/test/data/page_load_metrics/empty_iframe.html [modify] https://crrev.com/2701c48359353889f90c30309ff523273890cca7/chrome/test/data/page_load_metrics/iframe.html [modify] https://crrev.com/2701c48359353889f90c30309ff523273890cca7/chrome/test/data/page_load_metrics/iframes.html [modify] https://crrev.com/2701c48359353889f90c30309ff523273890cca7/chrome/test/data/page_load_metrics/main_frame_with_iframe.html [modify] https://crrev.com/2701c48359353889f90c30309ff523273890cca7/third_party/WebKit/Source/core/frame/FrameView.cpp [modify] https://crrev.com/2701c48359353889f90c30309ff523273890cca7/third_party/WebKit/Source/core/frame/FrameView.h [modify] https://crrev.com/2701c48359353889f90c30309ff523273890cca7/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp [modify] https://crrev.com/2701c48359353889f90c30309ff523273890cca7/third_party/WebKit/Source/core/paint/BUILD.gn [add] https://crrev.com/2701c48359353889f90c30309ff523273890cca7/third_party/WebKit/Source/core/paint/FramePaintTiming.h [modify] https://crrev.com/2701c48359353889f90c30309ff523273890cca7/third_party/WebKit/Source/core/paint/FramePainter.cpp [modify] https://crrev.com/2701c48359353889f90c30309ff523273890cca7/third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp [modify] https://crrev.com/2701c48359353889f90c30309ff523273890cca7/third_party/WebKit/Source/platform/graphics/paint/PaintController.h [modify] https://crrev.com/2701c48359353889f90c30309ff523273890cca7/third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp
,
Jun 5 2017
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by bmcquade@chromium.org
, Mar 27 2017