Bug 91766 – didFirstVisuallyNonEmptyLayout() callback not fired for initial about:blank page in new loading window

• Component: Layout and Rendering
• Status: RESOLVED WONTFIX

[Charles Reis]

Created attachment 153314 [details]
Repro page that writes into newly created tabs.

Based on the name, I would expect about:blank pages to not fire the didFirstVisuallyNonEmptyLayout() callback when loaded, since they are visually empty.  Instead, the callback should be fired at the time another window injects content into the about:blank page.

Currently, the callback is fired immediately when using window.open("").  It should instead fire if another window tries something like w.document.innerHTML += "foo" or w.document.write("foo").

Also surprisingly, the callback is not fired at all when using window.open(slow_url).  It appears that this is because FrameView::performPostLayoutTasks is being incorrectly called on the opener window, rather than the newly opened window.  As a result, m_firstVisuallyNonEmptyLayoutCallbackPending is false and the callback is not fired.

Simple repro page attached.  Just attach a debugger and listen to FrameLoader::didFirstVisuallyNonEmptyLayout.

[Adam Barth]

We need to understand the intent of this callback.  It's possible it's just part of the loading lifecycle and should always fire sometime after Commit and before DOMContentLoaded or Loaded.

> w.document.innerHTML += "foo"

It's not clear to me whether this should fire didFirstVisuallyNonEmptyLayout().  In this case, w might finish loading entirely before this line of code executes.

> the callback is not fired at all when using window.open(slow_url

That definitely sounds like a bug.  It should fire at some deterministic time in the lifecycle.

[Charles Reis]

(In reply to comment #1)
> We need to understand the intent of this callback.  It's possible it's just part of the loading lifecycle and should always fire sometime after Commit and before DOMContentLoaded or Loaded.

If this callback is intended to fire on unmodified about:blank pages, then I'm looking for a way to detect when an about:blank page is modified.  Any suggestions are welcome!

[Beth Dakin]

This is a very strange and poorly-named API. We have been considering re-naming it actually.

(In reply to comment #1)
> We need to understand the intent of this callback.  It's possible it's just part of the loading lifecycle and should always fire sometime after Commit and before DOMContentLoaded or Loaded.
> 
> > w.document.innerHTML += "foo"
> 

See these lines in FrameView::performPostLayoutTasks():

// Ensure that we always send this eventually.
if (!m_frame->document()->parsing() && m_frame->loader()->stateMachine()->committedFirstRealDocumentLoad())
            m_isVisuallyNonEmpty = true;

So yes, the assumption right now is that it should always fire when the above criteria are met.

> It's not clear to me whether this should fire didFirstVisuallyNonEmptyLayout().  In this case, w might finish loading entirely before this line of code executes.
> 
> > the callback is not fired at all when using window.open(slow_url
> 
> That definitely sounds like a bug.  It should fire at some deterministic time in the lifecycle.

This is probably a bug.

[Adam Barth]

Why do you care whether an about:blank page is modified?

[Charles Reis]

(In reply to comment #3)
> This is a very strange and poorly-named API. We have been considering re-naming it actually.
> 
> (In reply to comment #1)
> > We need to understand the intent of this callback.  It's possible it's just part of the loading lifecycle and should always fire sometime after Commit and before DOMContentLoaded or Loaded.
> > 
> > > w.document.innerHTML += "foo"
> > 
> 
> See these lines in FrameView::performPostLayoutTasks():
> 
> // Ensure that we always send this eventually.
> if (!m_frame->document()->parsing() && m_frame->loader()->stateMachine()->committedFirstRealDocumentLoad())
>             m_isVisuallyNonEmpty = true;
> 
> So yes, the assumption right now is that it should always fire when the above criteria are met.

Thanks, that clarifies things.  I've renamed this bug to be about the loading case.

For my use case (detecting when about:blank is no longer blank), I could perhaps listen to ChromeClient::layoutUpdated and then check whether the page is still empty.  Is there a sane way to check that?  Or a better event to listen to?

[Charles Reis]

(In reply to comment #4)
> Why do you care whether an about:blank page is modified?

That's the point at which it is no longer safe to show the pending URL in a newly created tab.  If you create a new tab with a slowly loading URL, we'd like to be able to display the URL in the address bar (http://crbug.com/9682).  However, a URL spoof exploit becomes possible if another window modifies the about:blank page before the slow page commits.

[Adam Barth]

> > Why do you care whether an about:blank page is modified?
> 
> That's the point at which it is no longer safe to show the pending URL in a newly created tab.  If you create a new tab with a slowly loading URL, we'd like to be able to display the URL in the address bar (http://crbug.com/9682).  However, a URL spoof exploit becomes possible if another window modifies the about:blank page before the slow page commits.

How is that different from
1) loading page A into a window X
2) Letting that load fully complete
3) Navigating X to page B (which is very slow)
4) Changing the contents of page A

In both cases, you'll be showing the pending URL for B and the content from A.

[Charles Reis]

(In reply to comment #7)
> > > Why do you care whether an about:blank page is modified?
> > 
> > That's the point at which it is no longer safe to show the pending URL in a newly created tab.  If you create a new tab with a slowly loading URL, we'd like to be able to display the URL in the address bar (http://crbug.com/9682).  However, a URL spoof exploit becomes possible if another window modifies the about:blank page before the slow page commits.
> 
> How is that different from
> 1) loading page A into a window X
> 2) Letting that load fully complete
> 3) Navigating X to page B (which is very slow)
> 4) Changing the contents of page A
> 
> In both cases, you'll be showing the pending URL for B and the content from A.

We don't show the pending URL if there's a committed URL in the window, so there's no risk.  That's also a much less common case, so it's ok leaving it that way.

Chrome currently just shows "about:blank" in the address bar in the "slow URL in new tab" case because of the spoof risk, but that's maddening to a lot of users (e.g., can't recover if the load fails).

I'm trying to make it possible to show the pending URL in a new tab, which is safe as long as no one is modifying the initial about:blank page.  This would be consistent with Opera's behavior.

[Adam Barth]

> We don't show the pending URL if there's a committed URL in the window, so there's no risk.  That's also a much less common case, so it's ok leaving it that way.

So, you only care about the initial document?  Blankness isn't really the thing you're after here.  You just want to hide the fact that WebCore uses an initial about:blank document in newly created windows.

> Chrome currently just shows "about:blank" in the address bar in the "slow URL in new tab" case because of the spoof risk, but that's maddening to a lot of users (e.g., can't recover if the load fails).

Right, that's the URL of the initial document.

[Adam Barth]

Possibly related bugs:

https://bugs.webkit.org/show_bug.cgi?id=64557
https://bugs.webkit.org/show_bug.cgi?id=77383

[Adam Barth]

Have you looked at dispatchDidNewFirstVisuallyNonEmptyLayout to see whether that works better for your use case?

[Charles Reis]

(In reply to comment #9)
> > We don't show the pending URL if there's a committed URL in the window, so there's no risk.  That's also a much less common case, so it's ok leaving it that way.
> 
> So, you only care about the initial document?  Blankness isn't really the thing you're after here.  You just want to hide the fact that WebCore uses an initial about:blank document in newly created windows.

Yes, I only care about the initial document.  But I think I do care whether it's still blank or not.  The spec allows the window's opener to modify the initial document (whether a load is in progress or not), and it's not safe to show the loading URL if the initial document has been modified.  The attached repro page demonstrates this.

I would like to show the loading URL until the initial document has been modified, but I haven't yet found a way to detect the modification.

(In reply to comment #11)
> Have you looked at dispatchDidNewFirstVisuallyNonEmptyLayout to see whether that works better for your use case?

Just took a look.  Good thought, but it isn't called when I modify the content of the initial document.  Other things I could check?

[Beth Dakin]

(In reply to comment #12)
> (In reply to comment #11)
> > Have you looked at dispatchDidNewFirstVisuallyNonEmptyLayout to see whether that works better for your use case?
> 
> Just took a look.  Good thought, but it isn't called when I modify the content of the initial document.  Other things I could check?

This heuristic will only kick in if you call Page::setRelevantRepaintedObjectsCounterThreshold(uint64_t threshold) to set a threshold for the number of relevant objects that have been painted. The code is in a very confusing state right now because the current implementation does not actually even use that number, it's just using the setting of that number to anything other than 0 as an indicator that the heuristic should run. See this comment in Page.cpp:

// FIXME: gPaintedObjectCounterThreshold is no longer used for calculating relevant repainted areas,
// and it should be removed. For the time being, it is useful because it allows us to avoid doing
// any of this work for ports that don't make sure of didNewFirstVisuallyNonEmptyLayout. We should
// remove this when we resolve <rdar://problem/10791680> Need to merge didFirstVisuallyNonEmptyLayout 
// and didNewFirstVisuallyNonEmptyLayout
static uint64_t gPaintedObjectCounterThreshold = 0;

Sorry so sloppy!

[Peter Kasting]

Charles, are you pushing this forward?  It's not clear who's supposed to make progress on this bug, and since it blocks http://crbug.com/9682 it would be good to fix somehow.

[Charles Reis]

(In reply to comment #14)
> Charles, are you pushing this forward?  It's not clear who's supposed to make progress on this bug, and since it blocks http://crbug.com/9682 it would be good to fix somehow.

It turns out Adam's right-- this is not the right signal for fixing http://crbug.com/9682.  Looking for visual changes is not sufficient for knowing when we have to stop showing the pending navigation's URL.

For example, code injected into the about:blank page could leave the page white but request a permission that shows a prompt/infobar.  We can't have the pending URL visible alongside the infobar.

It seems like we'll need some other signal to tell us that the initial document has somehow "become active" or assumed the security context of another window.  I don't know how that should look yet.

At any rate, this API apparently wasn't designed for the case I was looking for, so I'll mark this as WontFix.  We can further discuss the http://crbug.com/9682 problem on that bug.