Bug 74402 – didFirstVisuallyNonEmptyLayout() callback is not fired, when Page is restored from PageCache.

• Component: WebKit API
• Status: NEW

[alan]

didFirstVisuallyNonEmptyLayout() callback is not fired, when Page is restored from PageCache.

[alan]

Created attachment 119009 [details]
Patch

[alan]

As for the missing test case: looked at both internals.idl and WebKitTestRunner/InjectBundlePage to see where layout related event testing should go, but didnt find a place where it could fit. Any suggestion how to test this properly? Thanks.

[Simon Fraser (smfr)]

Comment on attachment 119009 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119009&action=review

> Source/WebCore/loader/FrameLoader.cpp:2034
> +    clear(true, true, false);

This would be a great time to change those booleans to enums.

[Brady Eidson]

(In reply to comment #2)
> As for the missing test case: looked at both internals.idl and WebKitTestRunner/InjectBundlePage to see where layout related event testing should go, but didnt find a place where it could fit. Any suggestion how to test this properly? Thanks.

This is not really an event such as "onload" or "pageshow", it's a delegate callback.

WebKitTestRunner need to implement the delegate callback and log it so it shows up in the test output.

InjectBundlePage.cpp has a comment showing where this delegate callback will go.

[alan]

Created attachment 127170 [details]
Patch

[WebKit Review Bot]

Attachment 127170 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Last 3072 characters of output:
0 master     -> origin/master  (forced update)
	M	LayoutTests/platform/chromium-linux/tables/mozilla/bugs/bug11026-expected.png
	M	LayoutTests/platform/chromium-linux/tables/mozilla/bugs/bug10565-expected.png
	M	LayoutTests/platform/chromium-linux/tables/mozilla/bugs/bug1188-expected.png
	M	LayoutTests/platform/chromium/test_expectations.txt
	A	LayoutTests/platform/chromium-mac-snowleopard/tables/mozilla/bugs/bug11026-expected.png
	A	LayoutTests/platform/chromium-mac-snowleopard/tables/mozilla/bugs/bug10565-expected.png
	M	LayoutTests/platform/chromium-mac-snowleopard/tables/mozilla/bugs/bug1188-expected.png
	D	LayoutTests/platform/chromium-mac/tables/mozilla/bugs/bug11026-expected.png
	D	LayoutTests/platform/chromium-mac/tables/mozilla/bugs/bug10565-expected.png
	M	LayoutTests/platform/chromium-mac-leopard/tables/mozilla/bugs/bug11026-expected.png
	M	LayoutTests/platform/chromium-mac-leopard/tables/mozilla/bugs/bug10565-expected.png
	M	LayoutTests/platform/chromium-mac-leopard/tables/mozilla/bugs/bug1188-expected.png
	M	LayoutTests/platform/chromium-win/tables/mozilla/bugs/bug1188-expected.png
	M	LayoutTests/platform/chromium-win/tables/mozilla/bugs/bug11026-expected.png
	M	LayoutTests/platform/chromium-win/tables/mozilla/bugs/bug10565-expected.png
	A	LayoutTests/inspector/elements/resolve-node-blocked-expected.txt
	A	LayoutTests/inspector/elements/resolve-node-blocked.html
	M	LayoutTests/ChangeLog
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/inspector/InjectedScript.cpp
	M	Source/WebCore/inspector/InjectedScript.h
W: -empty_dir: trunk/LayoutTests/platform/chromium-mac/tables/mozilla/bugs/bug10565-expected.png
W: -empty_dir: trunk/LayoutTests/platform/chromium-mac/tables/mozilla/bugs/bug11026-expected.png
r107808 = 41432d00ee5deb89df7c1d46c048e30e4ec715fb (refs/remotes/origin/master)
First, rewinding head to replay your work on top of it...
Applying: Web Inspector: crash when inspecting an element on a page with eval disabled by CSP
Using index info to reconstruct a base tree...
<stdin>:35: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebCore/inspector/InjectedScript.cpp
CONFLICT (content): Merge conflict in Source/WebCore/inspector/InjectedScript.cpp
Auto-merging Source/WebCore/inspector/InjectedScript.h
CONFLICT (content): Merge conflict in Source/WebCore/inspector/InjectedScript.h
Failed to merge in the changes.
Patch failed at 0001 Web Inspector: crash when inspecting an element on a page with eval disabled by CSP

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

I'm not sure if this is a bug. Logically, it's not the first non-empty layout for the page when it is restored.

The behavior of API callbacks when restoring a page from page cache is very strange - see e.g. bug 62740, but there were other issues I cannot find at the moment.

Can we agree on the behavior of other callbacks (like didFirstLayoutForFrame), and test it systemically?

[Brady Eidson]

I personally would like a new callback specifically about restoration from the page cache because - as Alexey mentions - this logically isn't the "first visually nonempty layout".  Logically there is usually *no* layout after restoring.

[alan]

(In reply to comment #7)
> I'm not sure if this is a bug. Logically, it's not the first non-empty layout for the page when it is restored.
FrameLoader does make an attempt to reset frameview and set the callback flags to default. It made me think that sending first non-empty layout, when restoring page, was the expected behavior. However, if we say that the current behavior is correct, history.forward() should not be sending it either (as it does with the current codebase)


> The behavior of API callbacks when restoring a page from page cache is very strange - see e.g. bug 62740, but there were other issues I cannot find at the moment.
> 
> Can we agree on the behavior of other callbacks (like didFirstLayoutForFrame), and test it systemically?

[Brady Eidson]

(In reply to comment #9)
> (In reply to comment #7)
> > I'm not sure if this is a bug. Logically, it's not the first non-empty layout for the page when it is restored.
> FrameLoader does make an attempt to reset frameview and set the callback flags to default. It made me think that sending first non-empty layout, when restoring page, was the expected behavior. However, if we say that the current behavior is correct, history.forward() should not be sending it either (as it does with the current codebase)

You're saying that history.forward() *WHEN* the forward page is in the page cache makes this call?

But history.back() *WHEN* the back page is in the page cache does not make this call?

That seems unlikely to me.

[Brady Eidson]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > I'm not sure if this is a bug. Logically, it's not the first non-empty layout for the page when it is restored.
> > FrameLoader does make an attempt to reset frameview and set the callback flags to default. It made me think that sending first non-empty layout, when restoring page, was the expected behavior. However, if we say that the current behavior is correct, history.forward() should not be sending it either (as it does with the current codebase)
> 
> You're saying that history.forward() *WHEN* the forward page is in the page cache makes this call?
> 
> But history.back() *WHEN* the back page is in the page cache does not make this call?
> 
> That seems unlikely to me.

If history.forward() in the page cache case is different from history.back() in the page cache case, then yes that's a bizarre inconsistency we should fix.

I still need to see evidence to believe it  :)

[alan]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > I'm not sure if this is a bug. Logically, it's not the first non-empty layout for the page when it is restored.
> > FrameLoader does make an attempt to reset frameview and set the callback flags to default. It made me think that sending first non-empty layout, when restoring page, was the expected behavior. However, if we say that the current behavior is correct, history.forward() should not be sending it either (as it does with the current codebase)
> 
> You're saying that history.forward() *WHEN* the forward page is in the page cache makes this call?
> 
> But history.back() *WHEN* the back page is in the page cache does not make this call?
> 
> That seems unlikely to me.

It's because FrameLoader::open() calls FrameLoader::clear() too soon, when the mainframe still holds the pointer to the current frameview instance. So we always reset the current frameview and not the one which is being restored.
Let's say you load page A and page B and then you do history.back() and history.forward(). 
When we are on Page B and history.back() is called, page B's frameview gets reset, so then later, when doing history.forward() the callback is called. (and similarly, while doing history.forward(), Page A's frameview gets reset, and then history.back() fires the callback this time)

[Brady Eidson]

(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #7)
> > > > I'm not sure if this is a bug. Logically, it's not the first non-empty layout for the page when it is restored.
> > > FrameLoader does make an attempt to reset frameview and set the callback flags to default. It made me think that sending first non-empty layout, when restoring page, was the expected behavior. However, if we say that the current behavior is correct, history.forward() should not be sending it either (as it does with the current codebase)
> > 
> > You're saying that history.forward() *WHEN* the forward page is in the page cache makes this call?
> > 
> > But history.back() *WHEN* the back page is in the page cache does not make this call?
> > 
> > That seems unlikely to me.
> 
> It's because FrameLoader::open() calls FrameLoader::clear() too soon, when the mainframe still holds the pointer to the current frameview instance. So we always reset the current frameview and not the one which is being restored.
> Let's say you load page A and page B and then you do history.back() and history.forward(). 
> When we are on Page B and history.back() is called, page B's frameview gets reset, so then later, when doing history.forward() the callback is called. (and similarly, while doing history.forward(), Page A's frameview gets reset, and then history.back() fires the callback this time)

I see.  So anytime you are navigating from any page X to any page Y that is in the page cache, and page X is eligible for the page cache, page X gets its FrameView reset.

It's not really about back/forward.

This sounds like a bug.  Again, by design the page is "paused" and looks exactly like it did when you left.  Logically there should be no layout.

[alan]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > (In reply to comment #9)
> > > > (In reply to comment #7)
> > > > > I'm not sure if this is a bug. Logically, it's not the first non-empty layout for the page when it is restored.
> > > > FrameLoader does make an attempt to reset frameview and set the callback flags to default. It made me think that sending first non-empty layout, when restoring page, was the expected behavior. However, if we say that the current behavior is correct, history.forward() should not be sending it either (as it does with the current codebase)
> > > 
> > > You're saying that history.forward() *WHEN* the forward page is in the page cache makes this call?
> > > 
> > > But history.back() *WHEN* the back page is in the page cache does not make this call?
> > > 
> > > That seems unlikely to me.
> > 
> > It's because FrameLoader::open() calls FrameLoader::clear() too soon, when the mainframe still holds the pointer to the current frameview instance. So we always reset the current frameview and not the one which is being restored.
> > Let's say you load page A and page B and then you do history.back() and history.forward(). 
> > When we are on Page B and history.back() is called, page B's frameview gets reset, so then later, when doing history.forward() the callback is called. (and similarly, while doing history.forward(), Page A's frameview gets reset, and then history.back() fires the callback this time)
> 
> I see.  So anytime you are navigating from any page X to any page Y that is in the page cache, and page X is eligible for the page cache, page X gets its FrameView reset.
> 
> It's not really about back/forward.
Yes, correct. I should have pointed out that history back/forward was just an easy way to reproduce this inconsistency, but it wasn't really about back/forward.

> 
> This sounds like a bug.  Again, by design the page is "paused" and looks exactly like it did when you left.  Logically there should be no layout.
Yes, most of the time there should be no layout. However, in some special (mobile) cases, when the content is sized to some fixed width (device-width) and the viewport gets resized (rotated), while the page is in the page cache, it does require layout. But I agree, it still does not justify sending off first non-empty layout again. 
I think, FrameView::reset() needs a close look to figure out, if any of those flags needs reset on page cache restore, and agree on the callbacks as Alexey pointed out.
and yes, I'd also like to see some callback on restoration.

[Luca Carlon]

I can reproduce this bug. In Qt, when going back, I can't get correctly both signals for the size of the content and of the new URL. Setting the cache to 0 pages max solved both the issues.

[Brady Eidson]

(In reply to comment #15)
> I can reproduce this bug. In Qt, when going back, I can't get correctly both signals for the size of the content and of the new URL. Setting the cache to 0 pages max solved both the issues.

Luca, please be very explicit about this.

What callback internal to WebCore ends up triggering the "Size of content" signal in Qt?

If you are saying that you are *expecting* to get didFirstVisuallyNonEmptyLayout when you return to a cached page and it's reproducible that you are *not* getting it...  that is not a bug.  didFirstVisuallyNonEmptyLayout happens for the first layout on a new page.  Period.

If you're expecting to get some other callback when you return to a cached page, please share what that callback is.

If you're saying "Qt needs a signal when returning to a cached page for the size-of-content and we're not getting one" then that's a port-specific bug for the Qt folks.

[alan]

(In reply to comment #16)
> (In reply to comment #15)
> > I can reproduce this bug. In Qt, when going back, I can't get correctly both signals for the size of the content and of the new URL. Setting the cache to 0 pages max solved both the issues.
> 
> Luca, please be very explicit about this.
> 
> What callback internal to WebCore ends up triggering the "Size of content" signal in Qt?
> 
> If you are saying that you are *expecting* to get didFirstVisuallyNonEmptyLayout when you return to a cached page and it's reproducible that you are *not* getting it...  that is not a bug.  didFirstVisuallyNonEmptyLayout happens for the first layout on a new page.  Period.
> 
> If you're expecting to get some other callback when you return to a cached page, please share what that callback is.
> 
> If you're saying "Qt needs a signal when returning to a cached page for the size-of-content and we're not getting one" then that's a port-specific bug for the Qt folks.
I'll make this bug Qt specific and open 2 bugs about 
1, WebCore should not send the non-empty delegate callback, when the page is being restored from page cache 
2, WebCore should send delegate callbacks on page restoration. 

After fixing those, which I am planning to do early next week, I can get back to this and fix it for Qt by using the page restoration callbacks.