Bug 53474 – PluginDocuments don't create widgets for plugins on back/forward

• Component: New Bugs
• Status: RESOLVED FIXED

[John Abd-El-Malek]

Don't cause a relayout when resetting the page scale factor is restored

[John Abd-El-Malek]

This fixes http://code.google.com/p/chromium/issues/detail?id=70046, which was caused by http://trac.webkit.org/changeset/75758.  That caused FrameView::layout to create the plugin objects later.  For full page plugins, PluginDocumentParser::appendBytes is then called before the plugin is created and so redirectDataToPlugin doesn't get to send the data to the plugin.  This may only impact Chromium because it doesn't have a back/forward cache.

[John Abd-El-Malek]

Created attachment 80718 [details]
Patch

[John Abd-El-Malek]

here's a proposed patch.  There might be better ways of fixing this, I'm not that familiar with the code.

[WebKit Review Bot]

Attachment 80718 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7681695

[David Levin]

Comment on attachment 80718 [details]
Patch

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

And this broke the mac build.

I'm not r- so that hopefully someone more familiar with this area will look and determine if this is a good approach or not (but there are several things that should to be fixed if this is the right approach).

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

This shouldn't be here. Either there should be a test or the ChangeLog should explain why a test isn't possible or necessary.

> Source/WebCore/ChangeLog:11
> +        (WebCore::HistoryController::restoreScrollPositionAndViewState):

Ideally there are some short/small comments per function which explain the change being done.

> Source/WebCore/page/Frame.cpp:999
> +        if (needsLayout && document->renderer())

"needsLayout &&" isn't needed here. It is checked for in the previous line.

> Source/WebCore/page/Frame.h:161
> +        void scalePage(float scale, const IntPoint& origin, bool needsLayout = true);

bool needsLayout should be an enum.

[Darin Adler]

Comment on attachment 80718 [details]
Patch

Why is it OK to not call setNeedsLayout in restoreScrollPositionAndViewState? What’s the guarantee that layout is OK? You’ll at least need a comment explaining why that’s true.

[John Abd-El-Malek]

(In reply to comment #6)
> (From update of attachment 80718 [details])
> Why is it OK to not call setNeedsLayout in restoreScrollPositionAndViewState? What’s the guarantee that layout is OK? You’ll at least need a comment explaining why that’s true.

Thanks for taking a look.  I was posting this patch to get feedback if it's the correct approach (I'm not that familiar with this code).

The problem with calling setNeedsLayout is that it ended up causing a full-page plugin later than normal.  This happens because in the nested FrameView::layout() call that now occurs, this statement:

  if (!m_hasPendingPostLayoutTasks && (needsLayout() || m_inSynchronousPostLayout)) {

now is true (because needsLayout() is true) and so m_hasPendingPostLayoutTasks is set to true.  When the stack unwinds to the original layout() call, the

  if (!m_hasPendingPostLayoutTasks) {

block now doesn't run, which includes the call to performPostLayoutTasks(), which is what creates the plugin through SubframeLoader::loadPlugin().

We then unwind the stack a little more and end up back in PluginDocumentParser::appendBytes.  It checks if m_embedElement->renderPart()->widget() is set, and if so, tells the frame loader client to redirectDataToPlugin() the data to it.  Now that the plugin isn't created, the data isn't set.

This problem doesn't repro with Safari, my guess is that because of the back/forth cache which Chromium doesn't use.  As such, full-page plugins in Chrome now don't get data when a user navigates back and forth.

[Adam Barth]

jam asked me to look at this bug.

[Adam Barth]

Created attachment 80839 [details]
Works but might be wrong

[John Abd-El-Malek]

(In reply to comment #9)
> Created an attachment (id=80839) [details]
> Works but might be wrong

agreed this looks much nicer.

if this approach is used, then the other code to update widget on layout should be removed.

[Adam Barth]

Created attachment 80843 [details]
Also works, might be better

[Adam Barth]

That boolean parameter was added in http://trac.webkit.org/changeset/25128 to delay instantiating plugins until after first layout so that they'll have some reasonable dimensions.

[Adam Barth]

Yep.  That patch also makes the test added in that patch fail:

--- /tmp/layout-test-results/plugins/netscape-plugin-setwindow-size-expected.txt	2011-02-01 16:16:32.000000000 -0800
+++ /tmp/layout-test-results/plugins/netscape-plugin-setwindow-size-actual.txt	2011-02-01 16:16:32.000000000 -0800
@@ -1,3 +1,3 @@
-CONSOLE MESSAGE: line 0: PLUGIN: NPP_SetWindow: 800 200
+CONSOLE MESSAGE: line 0: PLUGIN: NPP_SetWindow: 0 0
 
 This tests that a plug-in with a percentage width gets a correct NPP_SetWindow the first time.

[Darin Adler]

I think there is may be another real issue here. When restoring the page we should not call the scale function if the scale is already correct. At some point someone changed the scale function to do work even if there is nothing to do.

[John Abd-El-Malek]

(In reply to comment #14)
> I think there is may be another real issue here. When restoring the page we should not call the scale function if the scale is already correct. At some point someone changed the scale function to do work even if there is nothing to do.

That's right, but note that this won't fix the issue if a user navigates from a page to a plugin where the page has a different page scale, since this code path will get hit.  So we still need to solve this bug.  Not doing the unnecessary work is a nice optimization.

[Adam Barth]

This is an interesting bug.  The issue is that this change to the HistoryController causes use to call layout() from inside performPostLayoutTasks().  The code has logic to prevent overly nutty recursion by delaying the post-LayoutTasks using a timer.  Unfortunately, PluginDocument needs HTMLEmbedElement::updateWidget to be called synchronously so that the widget exists to redirect the bytes coming from the network.

It's slightly unclear to me how to fix this issue.  The approach in  attachment 80839 [details] works, but that will trigger the same issue that http://trac.webkit.org/changeset/25128 fixed, namely that we'll create the widget before we know how big it is...

Maybe the cleanest solution is to provide a mechanism to ask FrameView to flush its pending post-LayoutTasks.  That way we can avoid the recursion trap and still have those events take place before PluginDocument returns.

[John Abd-El-Malek]

(In reply to comment #16)
> This is an interesting bug.  The issue is that this change to the HistoryController causes use to call layout() from inside performPostLayoutTasks().  The code has logic to prevent overly nutty recursion by delaying the post-LayoutTasks using a timer.  Unfortunately, PluginDocument needs HTMLEmbedElement::updateWidget to be called synchronously so that the widget exists to redirect the bytes coming from the network.
> 
> It's slightly unclear to me how to fix this issue.  The approach in  attachment 80839 [details] works, but that will trigger the same issue that http://trac.webkit.org/changeset/25128 fixed, namely that we'll create the widget before we know how big it is...
> 
> Maybe the cleanest solution is to provide a mechanism to ask FrameView to flush its pending post-LayoutTasks.  That way we can avoid the recursion trap and still have those events take place before PluginDocument returns.

I wondered about the same things too, but wasn't sure what (if any) other post layout tasks do.  This can be done now by calling FrameView::postLayoutTimerFired() but that might not be the cleanest way :)

[John Abd-El-Malek]

(In reply to comment #17)
> (In reply to comment #16)
> > This is an interesting bug.  The issue is that this change to the HistoryController causes use to call layout() from inside performPostLayoutTasks().  The code has logic to prevent overly nutty recursion by delaying the post-LayoutTasks using a timer.  Unfortunately, PluginDocument needs HTMLEmbedElement::updateWidget to be called synchronously so that the widget exists to redirect the bytes coming from the network.
> > 
> > It's slightly unclear to me how to fix this issue.  The approach in  attachment 80839 [details] [details] works, but that will trigger the same issue that http://trac.webkit.org/changeset/25128 fixed, namely that we'll create the widget before we know how big it is...
> > 
> > Maybe the cleanest solution is to provide a mechanism to ask FrameView to flush its pending post-LayoutTasks.  That way we can avoid the recursion trap and still have those events take place before PluginDocument returns.
> 
> I wondered about the same things too, but wasn't sure what (if any) other post layout tasks do.  This can be done now by calling FrameView::postLayoutTimerFired() but that might not be the cleanest way :)

also, what about the first patch that I added, i.e. when scalePage is called, tell it not to do layout?

[Adam Barth]

Created attachment 80870 [details]
Might actually be a reasonable approach

[Adam Barth]

> also, what about the first patch that I added, i.e. when scalePage is called, tell it not to do layout?

That doesn't seem right to me, but I'm not an expert on layout.

[Adam Barth]

Created attachment 80881 [details]
Patch

[Eric Seidel (no email)]

So this patch makes it perform the post layout tests before they're scheduled to go.  Which seems error-prone.

The whole idea of having possibly async post layout tasks seems error prone.

[Eric Seidel (no email)]

Comment on attachment 80881 [details]
Patch

We talked about a possible way to make a test in person.  I think a test for this is more important than the code change if we can make one.

[John Abd-El-Malek]

(In reply to comment #23)
> (From update of attachment 80881 [details])
> We talked about a possible way to make a test in person.  I think a test for this is more important than the code change if we can make one.

guys, I'm all for a test for this, and a proper solution of course.  I'd just like to point out that this blocks Chrome's M10 release scheduled for 2/10 (the release, not when we make the build).  Perhaps we should put a temporary fix on the 648 branch?

[Adam Barth]

> guys, I'm all for a test for this, and a proper solution of course.  I'd just like to point out that this blocks Chrome's M10 release scheduled for 2/10 (the release, not when we make the build).  Perhaps we should put a temporary fix on the 648 branch?

I'll try another iteration today.  If we don't have this fixed by the end of the day, the above patch might be a reasonable candidate to land on the branch.  Generally speak, we prefer to land things on trunk first, and we also prefer writing better code than bending to schedule pressure.  Hopefully with some effort, we can satisfy all these desires.

[Adam Barth]

Looking at the clients of updateLayout, it seems that very few of them care about whether updateWidget has been called.  We can discuss it more, but I think the general approach of optionally asking for the postLayoutTasks to be called synchronously makes sense.

[Eric Seidel (no email)]

Beth recently worked on the post-layout-tasks timer:
https://bugs.webkit.org/show_bug.cgi?id=44828

[Eric Seidel (no email)]

Comment on attachment 80881 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:1824
> +void FrameView::flushAnyPendingPostLayoutTasks()

Probably this method should ASSERT that we're not in a synchronous post layout tasks call, or performPostLayoutTasks should.  Currently I believe neither do.

[Adam Barth]

Comment on attachment 80881 [details]
Patch

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

>> Source/WebCore/page/FrameView.cpp:1824
>> +void FrameView::flushAnyPendingPostLayoutTasks()
> 
> Probably this method should ASSERT that we're not in a synchronous post layout tasks call, or performPostLayoutTasks should.  Currently I believe neither do.

We could add the ASSERT here, but adding it to performPostLayoutTasks wouldn't work because performPostLayoutTasks is called immediately after setting that flag in layout.

[Eric Seidel (no email)]

Comment on attachment 80881 [details]
Patch

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

OK.  I think this is good as-is (with the extra comments), but we need a test to land.  I'm happy to help write said test.

> Source/WebCore/ChangeLog:17
> +

I think you should mention here the other approaches you tried and why they didn't work.

> Source/WebCore/ChangeLog:23
> +        I struggled for a while to write a test for this patch.  I figure out

I think we have an idea as to how and should do it before landing this.

> Source/WebCore/html/PluginDocument.cpp:124
> +    frame->view()->flushAnyPendingPostLayoutTasks();

I think this is the right approach.  However, we should add a FIXME that this will go away when we fix plugins to load via the DOM (instead of our current renderning nonsense).

> Source/WebCore/page/FrameView.h:257
> +    void flushAnyPendingPostLayoutTasks();

You should consider adding a note that this is only to be used by PluginDocument and that it can be removed once we support loading display:none plugins.

[Adam Barth]

Created attachment 81297 [details]
test in progress

[Adam Barth]

Created attachment 81306 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 81306 [details]
Patch

Thanks.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plug..." exit_code: 1

Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:203:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:203:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:204:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 9 files


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

[John Abd-El-Malek]

thanks Adam!

[Adam Barth]

Sorry it took so long.  This area of WebKit is very complex.  Over time its been modified by folks who didn't have complete understanding of how it works, which has lead to a lot of complexity.  In some regards, we'll also changing it without fully understanding the system, which is why we're trying to be cautious not to add more complexity than necessary.

[WebKit Commit Bot]

Comment on attachment 81306 [details]
Patch

Clearing flags on attachment: 81306

Committed r77706: <http://trac.webkit.org/changeset/77706>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/77706 might have broken GTK Linux 64-bit Debug