New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 615891 link

Starred by 1 user
Status: WontFix
Owner: ----
Closed: Dec 2016
Cc:
█ bmcquade@chromium.org
pdr@chromium.org
japhet@chromium.org
kinuko@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 236262



Sign in to add a comment

Page::didCommitLoad is invoked multiple times for most page loads

Project Member Reported by rbyers@chromium.org, May 30 2016 
I believe Page::didCommitLoad expects to be invoked once per frame as it starts loading, and most of it's logic expects to run exactly once per page load (for some concept of "page load"):

void Page::didCommitLoad(LocalFrame* frame)
{
    notifyDidCommitLoad(frame);
    if (m_mainFrame == frame) {
        frame->console().clearMessages();
        useCounter().didCommitLoad();
        deprecation().clearSuppression();
        frameHost().visualViewport().sendUMAMetrics();
        m_hostsUsingFeatures.updateMeasurementsAndClear();
        UserGestureIndicator::clearProcessedUserGestureSinceLoad();
    }
}

The particular case that's impacting me is  issue 236262 , but it looks like the other examples here are probably also affected.  Eg. when loading a page like http://jsbin.com, I see the logic inside the m_mainFrame==frame check get invoked 7 times.

Many of these appear to be due to SVG images - blink::SVGImage::dataChanged ultimately calls DocumentLoader::createWriterFor to create the SVG document writer, but that's using the same FrameLoader as the main document in the frame and so it's call to FrameLoader::receivedFirstData seems wrong (the frame has long ago received it's first data).

FrameLoader::didBeginDocument() seems likely affected by the same thing.

What's the right way to update this logic to run exactly once for each top-level navigation?  Brain can you please help / route?

Comment 1 by bmcquade@chromium.org, Jun 2 2016

Cc: kinuko@chromium.org bmcquade@chromium.org japhet@chromium.org
Owner: ----
Thanks for filing this! We've encountered some other cases where logic that's expected to get invoked once per page/document runs multiple times, for example issue 594677. This needs to get cleaned up but it's a big and fragile project.

I'm not sure I am a good owner here as I'm less familiar with blink object lifetimes. cc'ing kinuko and japhet who will know this code better.

My sense is that the simplest thing to do here may be to add a boolean member m_didCommitLoad, and gate the execution of your logic on that boolean being unset (and set it on the first commit). It's a bit of a hack but if we want to work around this issue in the near term without a bigger cleanup, I think that's the best way forward.

Comment 2 by rbyers@chromium.org, Jun 2 2016

Status: Available (was: Assigned)

Comment 3 Deleted

Comment 4 by rbyers@chromium.org, Jun 2 2016 

Thanks!  I can certainly add a hack like that.  I think Page instances are re-used across distinct navigations so their lifetime is too long, but perhaps it would make sense for FrameLoader to track such a bit?  I'll play with this a bit for  issue 236262  (but leave this issue for the larger problem here for loading-dev).

However I'm now leaning towards keeping our existing UseCounter metrics in place while we transition to more correct ones.  So whatever we do to "fix" this bug, we need to continue to invoke the logic in useCounter().didCommitLoad() according to the existing flawed behavior (although should probably rename the functions at least to make it clear that it's not what you'd expect).

Comment 5 by rbyers@chromium.org, Aug 25 2016

Cc: pdr@chromium.org
Note that I realized SVGImage is really weird in that it has it's own Page instance (which explains why each SVGImage results in a Page::didCommitLoad call).  I'm not sure which other cases I was seeing, I'll dig further...

Comment 6 by rbyers@chromium.org, Dec 23 2016

Status: WontFix (was: Available)
Ok, I think I've convinced myself that the only really strange case here is SVGImage.  There are loads from some other Page instances you might not always anticipate (plugin placeholders, workers, omnibox helpers).  But AFAICT there aren't any actual bugs.  See  issue 236262 .
►

Sign in to add a comment