Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2389023002:
Deferred frame loading stats v2 (Closed)
|
Created:
4 years, 2 months ago by dgrogan Modified:
4 years, 2 months ago CC:
chromium-reviews, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, sof, eae+blinkwatch, leviw+renderwatch, blink-reviews-dom_chromium.org, dglazkov+blink, jchaffraix+rendering, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLog to UMA why we would load a third-party frame
This will give an estimate of the % of iframes that we can defer
loading.
We can defer offscreen frames unless they are probably used for
cross-origin communication.
BUG=635105
Committed: https://crrev.com/89406fe9ba00bb4b543d3bb89da3d4682539a065
Cr-Commit-Position: refs/heads/master@{#426375}
Patch Set 1 #Patch Set 2 : remove extraneous changes #Patch Set 3 : display:none doesn't work #
Total comments: 4
Patch Set 4 : rebase #Patch Set 5 : make display none work. update histograms.xml #
Total comments: 15
Patch Set 6 : rebase #Patch Set 7 : address comments #
Total comments: 7
Patch Set 8 : address comments round 2 #
Total comments: 4
Patch Set 9 : fix up some tests; remove comment #
Total comments: 3
Patch Set 10 : move Created logging to LocalDOMWindow; rename wouldLoadBecause #
Total comments: 3
Messages
Total messages: 44 (18 generated)
Description was changed from ========== Visible doesn't work -- SimTest has 0x0 frame BUG= ========== to ========== Visible doesn't work -- SimTest has 0x0 frame BUG= ==========
Description was changed from ========== Visible doesn't work -- SimTest has 0x0 frame BUG= ========== to ========== Record more reasons for why we would load an unseen frame BUG=635105 ==========
dgrogan@chromium.org changed reviewers: + dcheng@chromium.org
Hi Daniel, Closer to ready-for-review but still need some help accessing a frame's style properties from FrameView (questions inline) Thanks for your help. https://codereview.chromium.org/2389023002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2389023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:4350: if (const ComputedStyle* style = element->computedStyle()) { I'm trying to detect when an iframe has style="display:none". But element->computedStyle() always returns nullptr. Any ideas how I can access the style from here? (It's also probably not a good sign that I'm using deprecated stuff.) https://codereview.chromium.org/2389023002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/DeferredLoadingTest.cpp (right): https://codereview.chromium.org/2389023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/DeferredLoadingTest.cpp:129: TEST_F(DeferredLoadingTest, DisplayNone) { This test fails.
dcheng@chromium.org changed reviewers: + timloh@chromium.org
Apologizes for the really delayed reply, that's unacceptable on my part. +timloh, who might have some ideas about the CSS questions here (sorry, my layout knowledge is seriously outdated, the last time I looked at this stuff, it was called RenderStyle) https://codereview.chromium.org/2389023002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2389023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:4348: // display:none frames have no parent because they're not in the DOM. I don't understand this comment in conjunction with the code below. I believe what happens with display:none iframes is they end up attached to the frame tree (this can be tested by appending a style="display:none" iframe to the DOM and observing that window.lenght) increases). However, it's not in the layout tree (see below) https://codereview.chromium.org/2389023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:4350: if (const ComputedStyle* style = element->computedStyle()) { On 2016/10/05 01:19:08, dgrogan wrote: > I'm trying to detect when an iframe has style="display:none". But > element->computedStyle() always returns nullptr. Any ideas how I can access the > style from here? (It's also probably not a good sign that I'm using deprecated > stuff.) I think what's happening here is that there's no LayoutObject, so ComputedStyle will always be null. Maybe not having a LayoutObject is a good enough proxy for display:none? +timloh who knows much more about CSS than me.
Thanks for the info. This patch is now ready to be reviewed. > Maybe not having a LayoutObject is a good enough proxy for display:none? +timloh > who knows much more about CSS than me. Ojan agrees that this proxy is probably good enough. Implemented.
The CQ bit was checked by dgrogan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Record more reasons for why we would load an unseen frame BUG=635105 ========== to ========== Log to UMA why we would load a third-party frame This will give a pretty accurate measure of the % of iframes for which we can defer loading. BUG=635105 ==========
Description was changed from ========== Log to UMA why we would load a third-party frame This will give a pretty accurate measure of the % of iframes for which we can defer loading. BUG=635105 ========== to ========== Log to UMA why we would load a third-party frame This will give an estimate of the % of iframes that we can defer loading. BUG=635105 ==========
Description was changed from ========== Log to UMA why we would load a third-party frame This will give an estimate of the % of iframes that we can defer loading. BUG=635105 ========== to ========== Log to UMA why we would load a third-party frame This will give an estimate of the % of iframes that we can defer loading. We can defer offscreen frames unless they are probably used for cross-origin communication. BUG=635105 ==========
Also, just for my understanding, it looks like we'll record this metric twice per page: once for created, and once with a reason that's not created. What's the purpose of this? Is this to give us an overall denominator for scaling the other would load reasons? https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:401: static void recordReasonToHistogram(WouldLoadReason reason) { Nit: recordLoadReasonToHistogram https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:520: if (frame() && frame()->tree().top()->securityContext() && Nit: Not introduced by this CL, but can this be frame() && frame()->isCrossOriginSubframe()? https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.h:238: // Logged to UMA, so don't re-arrange them without creating a new histogram. Nit: add a comment that describes the purpose of an enum. Perhaps some comments on the enum values would also be useful (for example, the purpose of WouldLoadOutOfProcess is not as clear to me) https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:4402: frame().document()->wouldLoadBecause(WouldLoadOutOfProcess); Should this be in an else? https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:4408: bool parentLoaded = parent->frame().document()->wouldLoadReason() != Created; Maybe add a comment here: it'll help me understand why we care about the parent frame's load reason being something other than created. I guess it's because we want to only "chain" down the reasons once the parent frame has proceeded past the Created state, but an explicit comment to that effect might be good. https://codereview.chromium.org/2389023002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2389023002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26995: + loading as many as possible. many => long
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/07 23:04:35, dgrogan wrote: > Thanks for the info. This patch is now ready to be reviewed. > > > Maybe not having a LayoutObject is a good enough proxy for display:none? > +timloh > > who knows much more about CSS than me. > > Ojan agrees that this proxy is probably good enough. Implemented. Sounds good to me.
Ready for another pass. This time with comments. https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:401: static void recordReasonToHistogram(WouldLoadReason reason) { On 2016/10/08 00:46:22, dcheng wrote: > Nit: recordLoadReasonToHistogram Done. https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:520: if (frame() && frame()->tree().top()->securityContext() && On 2016/10/08 00:46:22, dcheng wrote: > Nit: Not introduced by this CL, but can this be frame() && > frame()->isCrossOriginSubframe()? That would be nice but unfortunately not; LocalFrame::document() returns nullptr. The DCHECK this CL adds to LocalFrame.cpp fails with the stack below. [25406:25406:1012/152748:2351789511288:FATAL:LocalFrame.cpp(541)] Check failed: securityContext(). #0 0x7fe4542cbb4e base::debug::StackTrace::StackTrace() #1 0x7fe4543395ff logging::LogMessage::~LogMessage() #2 0x7fe4503a7e5d blink::LocalFrame::isCrossOriginSubframe() #3 0x7fe44ff30f5a blink::Document::Document() #4 0x7fe4504457f5 blink::HTMLDocument::HTMLDocument() #5 0x7fe44ff13aed blink::HTMLDocument::create() #6 0x7fe44ff1343d blink::DOMImplementation::createDocument() #7 0x7fe45038fd2b blink::LocalDOMWindow::createDocument() #8 0x7fe45038fe2c blink::LocalDOMWindow::installNewDocument() #9 0x7fe450a75f37 blink::DocumentLoader::createWriterFor() #10 0x7fe450a75bbf blink::DocumentLoader::ensureWriter() #11 0x7fe450a742f5 blink::DocumentLoader::commitData() #12 0x7fe450a73e27 blink::DocumentLoader::finishedLoading() #13 0x7fe450a76b14 blink::DocumentLoader::maybeLoadEmpty() #14 0x7fe450a76cf1 blink::DocumentLoader::startLoadingMainResource() #15 0x7fe450a94fd1 blink::FrameLoader::init() #16 0x7fe4548151fc blink::LocalFrame::init() #17 0x7fe4548cb493 blink::WebLocalFrameImpl::initializeCoreFrame() #18 0x7fe454913146 blink::WebViewImpl::setMainFrame() https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.h:238: // Logged to UMA, so don't re-arrange them without creating a new histogram. On 2016/10/08 00:46:22, dcheng wrote: > Nit: add a comment that describes the purpose of an enum. Perhaps some comments > on the enum values would also be useful (for example, the purpose of > WouldLoadOutOfProcess is not as clear to me) Done. https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:4402: frame().document()->wouldLoadBecause(WouldLoadOutOfProcess); On 2016/10/08 00:46:22, dcheng wrote: > Should this be in an else? It was ok without one because of the check in Document::wouldLoadBecause that only records to UMA on the first call. But it's clearer with the "else" so changed. https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:4408: bool parentLoaded = parent->frame().document()->wouldLoadReason() != Created; On 2016/10/08 00:46:22, dcheng wrote: > Maybe add a comment here: it'll help me understand why we care about the parent > frame's load reason being something other than created. I guess it's because we > want to only "chain" down the reasons once the parent frame has proceeded past > the Created state, but an explicit comment to that effect might be good. Comment added. I think we're saying the same thing but let me know if it's still not clear. Also, changed the != to > for easier understanding. https://codereview.chromium.org/2389023002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2389023002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26995: + loading as many as possible. On 2016/10/08 00:46:22, dcheng wrote: > many => long Done. https://codereview.chromium.org/2389023002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2389023002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4316: // And out-of-process frames are the only other frames that have no parent. Do you know if the line4316 comment is correct? Why might cross-origin frames not have a parent? https://codereview.chromium.org/2389023002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4326: bool parentLoaded = parent->frame().document()->wouldLoadReason() > Created; I suspect ">" makes the meaning clearer at first glance.
Just noticed this patch. A few drive-by questions (since I was somehow looking into the similar code path): is the plan trying to defer loading the main html file for x-origin frames until its visibility is determined, is that right? I thought that we won't layout until we load the main resource but are we changing the logic or I'm probably wrong? Also: when you start actually modifying the loading code would you include loading-reviews@ in the cc? Thanks!
https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:520: if (frame() && frame()->tree().top()->securityContext() && On 2016/10/13 01:06:21, dgrogan wrote: > On 2016/10/08 00:46:22, dcheng wrote: > > Nit: Not introduced by this CL, but can this be frame() && > > frame()->isCrossOriginSubframe()? > > That would be nice but unfortunately not; LocalFrame::document() returns > nullptr. The DCHECK this CL adds to LocalFrame.cpp fails with the stack below. > [25406:25406:1012/152748:2351789511288:FATAL:LocalFrame.cpp(541)] Check failed: > securityContext(). > #0 0x7fe4542cbb4e base::debug::StackTrace::StackTrace() > #1 0x7fe4543395ff logging::LogMessage::~LogMessage() > #2 0x7fe4503a7e5d blink::LocalFrame::isCrossOriginSubframe() > #3 0x7fe44ff30f5a blink::Document::Document() > #4 0x7fe4504457f5 blink::HTMLDocument::HTMLDocument() > #5 0x7fe44ff13aed blink::HTMLDocument::create() > #6 0x7fe44ff1343d blink::DOMImplementation::createDocument() > #7 0x7fe45038fd2b blink::LocalDOMWindow::createDocument() > #8 0x7fe45038fe2c blink::LocalDOMWindow::installNewDocument() > #9 0x7fe450a75f37 blink::DocumentLoader::createWriterFor() > #10 0x7fe450a75bbf blink::DocumentLoader::ensureWriter() > #11 0x7fe450a742f5 blink::DocumentLoader::commitData() > #12 0x7fe450a73e27 blink::DocumentLoader::finishedLoading() > #13 0x7fe450a76b14 blink::DocumentLoader::maybeLoadEmpty() > #14 0x7fe450a76cf1 blink::DocumentLoader::startLoadingMainResource() > #15 0x7fe450a94fd1 blink::FrameLoader::init() > #16 0x7fe4548151fc blink::LocalFrame::init() > #17 0x7fe4548cb493 blink::WebLocalFrameImpl::initializeCoreFrame() > #18 0x7fe454913146 blink::WebViewImpl::setMainFrame() Let's just update isCrossOriginSubframe() to handle this case: returning false immediately when isMainFrame is true should handle this, I think. https://codereview.chromium.org/2389023002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2389023002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:246: // if the inner frame is visible, so just load it. Maybe add a TODO pointing to https://crbug.com/650433 here: we do plan on fixing this eventually. https://codereview.chromium.org/2389023002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2389023002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4319: else Right now, this will record WouldLoadOutOfProcess if an iframe's parent is in-process and has a layout object (since it doesn't meet both of the first conditions) Let's restructure this to: // Determining visibility uses the owner element's LayoutObject, // which is not currently possible if the parent is // remote. if (!element) wouldLoadBecause(WouldLoadOutOfProcess) // Having no layout object means the frame is not drawn. else if (!element->layoutObject()) wouldLoadBecause(WouldLoadDisplayNone) which also puts each comment near the the clause it documents.
Patchset #8 (id:140001) has been deleted
Kinuko's questions: (1) is the plan trying to defer loading the main html file for x-origin frames until its visibility is determined, is that right? Right. (2) I thought that we won't layout until we load the main resource but are we changing the logic or I'm probably wrong? I am not yet familiar with the existing behavior; this code, me, and gdb have a long road ahead of us. Do you mean we don't layout the outer page until the inner frame's main resource is loaded? That would seem weird but I am probably misunderstanding you. https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:520: if (frame() && frame()->tree().top()->securityContext() && On 2016/10/13 23:00:43, dcheng wrote: > On 2016/10/13 01:06:21, dgrogan wrote: > > On 2016/10/08 00:46:22, dcheng wrote: > > > Nit: Not introduced by this CL, but can this be frame() && > > > frame()->isCrossOriginSubframe()? > > > > That would be nice but unfortunately not; LocalFrame::document() returns > > nullptr. The DCHECK this CL adds to LocalFrame.cpp fails with the stack below. > > [25406:25406:1012/152748:2351789511288:FATAL:LocalFrame.cpp(541)] Check > failed: > > securityContext(). > > #0 0x7fe4542cbb4e base::debug::StackTrace::StackTrace() > > #1 0x7fe4543395ff logging::LogMessage::~LogMessage() > > #2 0x7fe4503a7e5d blink::LocalFrame::isCrossOriginSubframe() > > #3 0x7fe44ff30f5a blink::Document::Document() > > #4 0x7fe4504457f5 blink::HTMLDocument::HTMLDocument() > > #5 0x7fe44ff13aed blink::HTMLDocument::create() > > #6 0x7fe44ff1343d blink::DOMImplementation::createDocument() > > #7 0x7fe45038fd2b blink::LocalDOMWindow::createDocument() > > #8 0x7fe45038fe2c blink::LocalDOMWindow::installNewDocument() > > #9 0x7fe450a75f37 blink::DocumentLoader::createWriterFor() > > #10 0x7fe450a75bbf blink::DocumentLoader::ensureWriter() > > #11 0x7fe450a742f5 blink::DocumentLoader::commitData() > > #12 0x7fe450a73e27 blink::DocumentLoader::finishedLoading() > > #13 0x7fe450a76b14 blink::DocumentLoader::maybeLoadEmpty() > > #14 0x7fe450a76cf1 blink::DocumentLoader::startLoadingMainResource() > > #15 0x7fe450a94fd1 blink::FrameLoader::init() > > #16 0x7fe4548151fc blink::LocalFrame::init() > > #17 0x7fe4548cb493 blink::WebLocalFrameImpl::initializeCoreFrame() > > #18 0x7fe454913146 blink::WebViewImpl::setMainFrame() > > Let's just update isCrossOriginSubframe() to handle this case: returning false > immediately when isMainFrame is true should handle this, I think. Same stacktrace :( But thanks for looking into this, the reimplementation of that function in this constructor is a terrible smell. I'm completely hand-waving but it's like the localframe doesn't know about the new document yet. https://codereview.chromium.org/2389023002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2389023002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:246: // if the inner frame is visible, so just load it. On 2016/10/13 23:00:43, dcheng wrote: > Maybe add a TODO pointing to https://crbug.com/650433 here: we do plan on fixing > this eventually. Done. https://codereview.chromium.org/2389023002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2389023002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4319: else On 2016/10/13 23:00:43, dcheng wrote: > Right now, this will record WouldLoadOutOfProcess if an iframe's parent is > in-process and has a layout object (since it doesn't meet both of the first > conditions) If an iframe's parent is in-process we wouldn't enter this block because of the !parent check above? > Let's restructure this to: > > // Determining visibility uses the owner element's LayoutObject, > // which is not currently possible if the parent is > // remote. > if (!element) > wouldLoadBecause(WouldLoadOutOfProcess) > // Having no layout object means the frame is not drawn. > else if (!element->layoutObject()) > wouldLoadBecause(WouldLoadDisplayNone) > > which also puts each comment near the the clause it documents. Do we need to worry about the "else" case? What could hit it? I added "else NOTREACHED()" and 3 layout tests hit it but I don't see what's common between them except perhaps objects/iframes being unloaded. https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/dom/... https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/load... https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/imported/...
https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:520: if (frame() && frame()->tree().top()->securityContext() && On 2016/10/14 21:45:08, dgrogan wrote: > On 2016/10/13 23:00:43, dcheng wrote: > > On 2016/10/13 01:06:21, dgrogan wrote: > > > On 2016/10/08 00:46:22, dcheng wrote: > > > > Nit: Not introduced by this CL, but can this be frame() && > > > > frame()->isCrossOriginSubframe()? > > > > > > That would be nice but unfortunately not; LocalFrame::document() returns > > > nullptr. The DCHECK this CL adds to LocalFrame.cpp fails with the stack > below. > > > [25406:25406:1012/152748:2351789511288:FATAL:LocalFrame.cpp(541)] Check > > failed: > > > securityContext(). > > > #0 0x7fe4542cbb4e base::debug::StackTrace::StackTrace() > > > #1 0x7fe4543395ff logging::LogMessage::~LogMessage() > > > #2 0x7fe4503a7e5d blink::LocalFrame::isCrossOriginSubframe() > > > #3 0x7fe44ff30f5a blink::Document::Document() > > > #4 0x7fe4504457f5 blink::HTMLDocument::HTMLDocument() > > > #5 0x7fe44ff13aed blink::HTMLDocument::create() > > > #6 0x7fe44ff1343d blink::DOMImplementation::createDocument() > > > #7 0x7fe45038fd2b blink::LocalDOMWindow::createDocument() > > > #8 0x7fe45038fe2c blink::LocalDOMWindow::installNewDocument() > > > #9 0x7fe450a75f37 blink::DocumentLoader::createWriterFor() > > > #10 0x7fe450a75bbf blink::DocumentLoader::ensureWriter() > > > #11 0x7fe450a742f5 blink::DocumentLoader::commitData() > > > #12 0x7fe450a73e27 blink::DocumentLoader::finishedLoading() > > > #13 0x7fe450a76b14 blink::DocumentLoader::maybeLoadEmpty() > > > #14 0x7fe450a76cf1 blink::DocumentLoader::startLoadingMainResource() > > > #15 0x7fe450a94fd1 blink::FrameLoader::init() > > > #16 0x7fe4548151fc blink::LocalFrame::init() > > > #17 0x7fe4548cb493 blink::WebLocalFrameImpl::initializeCoreFrame() > > > #18 0x7fe454913146 blink::WebViewImpl::setMainFrame() > > > > Let's just update isCrossOriginSubframe() to handle this case: returning false > > immediately when isMainFrame is true should handle this, I think. > > Same stacktrace :( But thanks for looking into this, the reimplementation of > that function in this constructor is a terrible smell. > > I'm completely hand-waving but it's like the localframe doesn't know about the > new document yet. Ah OK. So I patched this in and I understand the problem: it's a chicken-and-egg problem. When a frame is first created, it has no document attached. Thus, LocalFrame::securityContext() will return null. When we create the initial empty document for that frame and call isCrossOriginSubframe(), it's unable to say. If we move this check after m_document is set in LocalDOMWindow::installNewDocument(), then it will work. I guess my question is: how long do we expect this code to live in tree? Indefinitely? or just for some time while we gather stats? The cleaner way is to probably just put it in installNewDocument(), but that requires exposing more stuff on Document. https://codereview.chromium.org/2389023002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2389023002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4319: else On 2016/10/14 21:45:08, dgrogan wrote: > On 2016/10/13 23:00:43, dcheng wrote: > > Right now, this will record WouldLoadOutOfProcess if an iframe's parent is > > in-process and has a layout object (since it doesn't meet both of the first > > conditions) > > If an iframe's parent is in-process we wouldn't enter this block because of the > !parent check above? Ah, I misread this originally. So I would expect parentFrameView() to only be null for two cases: - |this| is the main frame - |this| is a child frame with a remote parent. Taking that into account, I think the conditions we need to check for display:none should be (I believe display:none iframes still have a FrameView): if (parentFrameView() && !frame().deprecatedLocalOwner().layoutObject()) wouldLoadBecause(DisplayNone); And then for the remote frame case: else if (frame->owner() && frame->owner()->isRemote()) { wouldLoadBecause(OutOfProcess); } Does this match the desired semantics? > > > Let's restructure this to: > > > > // Determining visibility uses the owner element's LayoutObject, > > // which is not currently possible if the parent is > > // remote. > > if (!element) > > wouldLoadBecause(WouldLoadOutOfProcess) > > // Having no layout object means the frame is not drawn. > > else if (!element->layoutObject()) > > wouldLoadBecause(WouldLoadDisplayNone) > > > > which also puts each comment near the the clause it documents. > > Do we need to worry about the "else" case? What could hit it? I added "else > NOTREACHED()" and 3 layout tests hit it but I don't see what's common between > them except perhaps objects/iframes being unloaded. > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/dom/... > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/load... > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/imported/... probably not, but it's hard to say for sure =/ https://codereview.chromium.org/2389023002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2389023002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalFrame.cpp:543: DCHECK(securityContext()); Let's revert these 3 lines.
Description was changed from ========== Log to UMA why we would load a third-party frame This will give an estimate of the % of iframes that we can defer loading. We can defer offscreen frames unless they are probably used for cross-origin communication. BUG=635105 ========== to ========== Log to UMA why we would load a third-party frame This will give an estimate of the % of iframes that we can defer loading. We can defer offscreen frames unless they are probably used for cross-origin communication. BUG=635105 ==========
On 2016/10/14 21:45:08, dgrogan wrote: > Kinuko's questions: > > (2) I thought that we won't layout until we load the main resource but are we > changing the logic or I'm probably wrong? > > I am not yet familiar with the existing behavior; this code, me, and gdb have a > long road ahead of us. Do you mean we don't layout the outer page until the > inner frame's main resource is loaded? That would seem weird but I am probably > misunderstanding you. Thanks- well, when I have tried to hack up the similar code before to do the same/similar thing I observed that FrameView::notifyRenderThrottlingObservers() (which is where you're ) is not called for the frame if we stop or defer loading main resource for the frame (I did this in ResourceFetcher layer), so I presumed that the frame layout would depend on loading the main resource. I haven't really investigated the code though.
Thanks for the review, answers inline. On 2016/10/14 23:40:30, dcheng wrote: > https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/Document.cpp (right): > > https://codereview.chromium.org/2389023002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/Document.cpp:520: if (frame() && > frame()->tree().top()->securityContext() && > On 2016/10/14 21:45:08, dgrogan wrote: > > On 2016/10/13 23:00:43, dcheng wrote: > > > On 2016/10/13 01:06:21, dgrogan wrote: > > > > On 2016/10/08 00:46:22, dcheng wrote: > > > > > Nit: Not introduced by this CL, but can this be frame() && > > > > > frame()->isCrossOriginSubframe()? > > > > > > > > That would be nice but unfortunately not; LocalFrame::document() returns > > > > nullptr. The DCHECK this CL adds to LocalFrame.cpp fails with the stack > > below. > > > > [25406:25406:1012/152748:2351789511288:FATAL:LocalFrame.cpp(541)] Check > > > failed: > > > > securityContext(). > > > > #0 0x7fe4542cbb4e base::debug::StackTrace::StackTrace() > > > > #1 0x7fe4543395ff logging::LogMessage::~LogMessage() > > > > #2 0x7fe4503a7e5d blink::LocalFrame::isCrossOriginSubframe() > > > > #3 0x7fe44ff30f5a blink::Document::Document() > > > > #4 0x7fe4504457f5 blink::HTMLDocument::HTMLDocument() > > > > #5 0x7fe44ff13aed blink::HTMLDocument::create() > > > > #6 0x7fe44ff1343d blink::DOMImplementation::createDocument() > > > > #7 0x7fe45038fd2b blink::LocalDOMWindow::createDocument() > > > > #8 0x7fe45038fe2c blink::LocalDOMWindow::installNewDocument() > > > > #9 0x7fe450a75f37 blink::DocumentLoader::createWriterFor() > > > > #10 0x7fe450a75bbf blink::DocumentLoader::ensureWriter() > > > > #11 0x7fe450a742f5 blink::DocumentLoader::commitData() > > > > #12 0x7fe450a73e27 blink::DocumentLoader::finishedLoading() > > > > #13 0x7fe450a76b14 blink::DocumentLoader::maybeLoadEmpty() > > > > #14 0x7fe450a76cf1 blink::DocumentLoader::startLoadingMainResource() > > > > #15 0x7fe450a94fd1 blink::FrameLoader::init() > > > > #16 0x7fe4548151fc blink::LocalFrame::init() > > > > #17 0x7fe4548cb493 blink::WebLocalFrameImpl::initializeCoreFrame() > > > > #18 0x7fe454913146 blink::WebViewImpl::setMainFrame() > > > > > > Let's just update isCrossOriginSubframe() to handle this case: returning > false > > > immediately when isMainFrame is true should handle this, I think. > > > > Same stacktrace :( But thanks for looking into this, the reimplementation of > > that function in this constructor is a terrible smell. > > > > I'm completely hand-waving but it's like the localframe doesn't know about the > > new document yet. > > Ah OK. So I patched this in and I understand the problem: it's a chicken-and-egg > problem. > > When a frame is first created, it has no document attached. Thus, > LocalFrame::securityContext() will return null. When we create the initial empty > document for that frame and call isCrossOriginSubframe(), it's unable to say. > > If we move this check after m_document is set in > LocalDOMWindow::installNewDocument(), then it will work. > > I guess my question is: how long do we expect this code to live in tree? > Indefinitely? or just for some time while we gather stats? Just for some time while we gather stats. The cleaner way is to > probably just put it in installNewDocument(), but that requires exposing more > stuff on Document. > > https://codereview.chromium.org/2389023002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/frame/FrameView.cpp (right): > > https://codereview.chromium.org/2389023002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/frame/FrameView.cpp:4319: else > On 2016/10/14 21:45:08, dgrogan wrote: > > On 2016/10/13 23:00:43, dcheng wrote: > > > Right now, this will record WouldLoadOutOfProcess if an iframe's parent is > > > in-process and has a layout object (since it doesn't meet both of the first > > > conditions) > > > > If an iframe's parent is in-process we wouldn't enter this block because of > the > > !parent check above? > > Ah, I misread this originally. So I would expect parentFrameView() to only be > null for two cases: > - |this| is the main frame > - |this| is a child frame with a remote parent. This appears to not be the case; parentFrameView() returns null for the display:none frame too. (Otherwise the new test DeferredLoadingTest.DisplayNone would fail but it passes with the code as is.) FrameView* FrameView::parentFrameView() const { if (!parent()) return nullptr; // <--- display:none triggers this case ... } But I tried your earlier suggestion of attaching a display:none iframe to the DOM and it indeed causes window.length to increase: https://jsfiddle.net/dgrogan/86Lodjoh/ > Taking that into account, I think the conditions we need to check for > display:none should be (I believe display:none iframes still have a FrameView): > > if (parentFrameView() && !frame().deprecatedLocalOwner().layoutObject()) > wouldLoadBecause(DisplayNone); > > And then for the remote frame case: > > else if (frame->owner() && frame->owner()->isRemote()) { > wouldLoadBecause(OutOfProcess); > } > > Does this match the desired semantics? > > > > > > Let's restructure this to: > > > > > > // Determining visibility uses the owner element's LayoutObject, > > > // which is not currently possible if the parent is > > > // remote. > > > if (!element) > > > wouldLoadBecause(WouldLoadOutOfProcess) > > > // Having no layout object means the frame is not drawn. > > > else if (!element->layoutObject()) > > > wouldLoadBecause(WouldLoadDisplayNone) > > > > > > which also puts each comment near the the clause it documents. > > > > Do we need to worry about the "else" case? What could hit it? I added "else > > NOTREACHED()" and 3 layout tests hit it but I don't see what's common between > > them except perhaps objects/iframes being unloaded. > > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/dom/... > > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/load... > > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/imported/... > > probably not, but it's hard to say for sure =/ > > https://codereview.chromium.org/2389023002/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): > > https://codereview.chromium.org/2389023002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/frame/LocalFrame.cpp:543: > DCHECK(securityContext()); > Let's revert these 3 lines.
Thanks for the explanations. The display:none info has been surprising to me. LGTM with comments addressed (assuming this works) and reverting the no-longer needed changes in LocalFrame.cpp. https://codereview.chromium.org/2389023002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2389023002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:523: frame()->loader().stateMachine()->committedFirstRealDocumentLoad()) { Let's just do this check first; then I think we can just reuse isCrossOriginSubframe().
https://codereview.chromium.org/2389023002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2389023002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4315: // Determining visibility uses the owner element's LayoutObject, I used this comment without thinking about it, but it doesn't seem to be true. We're using the FrameView's frameRect(), not the LayoutObject. https://codereview.chromium.org/2389023002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2389023002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalFrame.cpp:543: DCHECK(securityContext()); On 2016/10/14 23:40:30, dcheng wrote: > Let's revert these 3 lines. Done. https://codereview.chromium.org/2389023002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2389023002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:522: frame()->tree().top()->securityContext() && Even checking committedFirstRealDocumentLoad() first, changing this to frame()->isCrossOriginSubframe() still dies with the same null pointer deref.
dgrogan@chromium.org changed reviewers: + isherman@chromium.org
isherman, can you review the histograms?
LGTM, though it's not quite clear to me why you're renaming the histogram. (If you don't mind the data discontinuity, though, there's no harm to renaming.)
https://codereview.chromium.org/2389023002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2389023002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:522: frame()->tree().top()->securityContext() && On 2016/10/18 22:11:45, dgrogan wrote: > Even checking committedFirstRealDocumentLoad() first, changing this to > frame()->isCrossOriginSubframe() still dies with the same null pointer deref. OK, yeah, I think this logic should really just live in installNewDocument().
Thanks for the reviews. Daniel, another look? Most of the interdiff is a method rename. To Ilya's question: Renaming the histogram for a few reasons. One is because I'm changing what WouldLoadVisible means. It used to mean that the frame is visible or has 0x0 size (weird, right?). It now means only that the frame is visible; 0x0 is split out into its own bucket. Another is that Created/WouldLoadVisible were recorded twice for each document in V1 and once for each document in V2. Combining those might give us some biased data, I think. Anyway, dealing with the discontinuity is easier than dealing with the change in meaning. https://codereview.chromium.org/2389023002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2389023002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:522: frame()->tree().top()->securityContext() && On 2016/10/19 01:34:54, dcheng wrote: > On 2016/10/18 22:11:45, dgrogan wrote: > > Even checking committedFirstRealDocumentLoad() first, changing this to > > frame()->isCrossOriginSubframe() still dies with the same null pointer deref. > > OK, yeah, I think this logic should really just live in installNewDocument(). Done.
On 2016/10/19 18:59:54, dgrogan wrote: > To Ilya's question: > > Renaming the histogram for a few reasons. One is because I'm changing what > WouldLoadVisible means. It used to mean that the frame is visible or has 0x0 > size (weird, right?). It now means only that the frame is visible; 0x0 is split > out into its own bucket. > > Another is that Created/WouldLoadVisible were recorded twice for each document > in V1 and once for each document in V2. Combining those might give us some > biased data, I think. > > Anyway, dealing with the discontinuity is easier than dealing with the change in > meaning. Yep, those are both good reasons to rename. Thanks!
LGTM with one comment https://codereview.chromium.org/2389023002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2389023002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:353: m_document->maybeRecordLoadReason(Created); Does this need to be in a if (frame()->isCrossOriginSubframe()) block?
https://codereview.chromium.org/2389023002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2389023002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:353: m_document->maybeRecordLoadReason(Created); On 2016/10/19 22:27:45, dcheng wrote: > Does this need to be in a if (frame()->isCrossOriginSubframe()) block? Nope, maybeRecordLoadReason does that check.
https://codereview.chromium.org/2389023002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2389023002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:353: m_document->maybeRecordLoadReason(Created); On 2016/10/19 22:31:40, dgrogan wrote: > On 2016/10/19 22:27:45, dcheng wrote: > > Does this need to be in a if (frame()->isCrossOriginSubframe()) block? > > Nope, maybeRecordLoadReason does that check. Ah, sorry for missing that!
The CQ bit was checked by dgrogan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2389023002/#ps200001 (title: "move Created logging to LocalDOMWindow; rename wouldLoadBecause")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Log to UMA why we would load a third-party frame This will give an estimate of the % of iframes that we can defer loading. We can defer offscreen frames unless they are probably used for cross-origin communication. BUG=635105 ========== to ========== Log to UMA why we would load a third-party frame This will give an estimate of the % of iframes that we can defer loading. We can defer offscreen frames unless they are probably used for cross-origin communication. BUG=635105 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
On 2016/10/20 02:06:39, commit-bot: I haz the power wrote: > Committed patchset #10 (id:200001) Hi, a revert of this patchset has been committed. See https://codereview.chromium.org/2432933005/ for details.
Message was sent while issue was closed.
Description was changed from ========== Log to UMA why we would load a third-party frame This will give an estimate of the % of iframes that we can defer loading. We can defer offscreen frames unless they are probably used for cross-origin communication. BUG=635105 ========== to ========== Log to UMA why we would load a third-party frame This will give an estimate of the % of iframes that we can defer loading. We can defer offscreen frames unless they are probably used for cross-origin communication. BUG=635105 Committed: https://crrev.com/89406fe9ba00bb4b543d3bb89da3d4682539a065 Cr-Commit-Position: refs/heads/master@{#426375} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/89406fe9ba00bb4b543d3bb89da3d4682539a065 Cr-Commit-Position: refs/heads/master@{#426375} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
