Bug 53474 - PluginDocuments don't create widgets for plugins on back/forward
Summary: PluginDocuments don't create widgets for plugins on back/forward
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on: 53549
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-31 22:08 PST by John Abd-El-Malek
Modified: 2011-02-04 18:32 PST (History)
8 users (show)

See Also:


Attachments
Patch (2.99 KB, patch)
2011-01-31 22:32 PST, John Abd-El-Malek
no flags Details | Formatted Diff | Diff
Works but might be wrong (627 bytes, patch)
2011-02-01 15:38 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Also works, might be better (544 bytes, patch)
2011-02-01 15:53 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Might actually be a reasonable approach (1.92 KB, patch)
2011-02-01 18:18 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (4.58 KB, patch)
2011-02-01 20:44 PST, Adam Barth
no flags Details | Formatted Diff | Diff
test in progress (6.63 KB, patch)
2011-02-04 15:06 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (8.11 KB, patch)
2011-02-04 15:34 PST, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Abd-El-Malek 2011-01-31 22:08:17 PST
Don't cause a relayout when resetting the page scale factor is restored
Comment 1 John Abd-El-Malek 2011-01-31 22:31:35 PST
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.
Comment 2 John Abd-El-Malek 2011-01-31 22:32:01 PST
Created attachment 80718 [details]
Patch
Comment 3 John Abd-El-Malek 2011-01-31 22:33:50 PST
here's a proposed patch.  There might be better ways of fixing this, I'm not that familiar with the code.
Comment 4 WebKit Review Bot 2011-02-01 02:14:26 PST
Attachment 80718 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7681695
Comment 5 David Levin 2011-02-01 09:03:46 PST
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.
Comment 6 Darin Adler 2011-02-01 10:02:24 PST
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.
Comment 7 John Abd-El-Malek 2011-02-01 10:44:59 PST
(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.
Comment 8 Adam Barth 2011-02-01 11:51:25 PST
jam asked me to look at this bug.
Comment 9 Adam Barth 2011-02-01 15:38:49 PST
Created attachment 80839 [details]
Works but might be wrong
Comment 10 John Abd-El-Malek 2011-02-01 15:47:31 PST
(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.
Comment 11 Adam Barth 2011-02-01 15:53:14 PST
Created attachment 80843 [details]
Also works, might be better
Comment 12 Adam Barth 2011-02-01 16:02:55 PST
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.
Comment 13 Adam Barth 2011-02-01 16:17:18 PST
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.
Comment 14 Darin Adler 2011-02-01 17:17:52 PST
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.
Comment 15 John Abd-El-Malek 2011-02-01 17:26:26 PST
(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.
Comment 16 Adam Barth 2011-02-01 17:56:33 PST
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.
Comment 17 John Abd-El-Malek 2011-02-01 18:01:14 PST
(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 :)
Comment 18 John Abd-El-Malek 2011-02-01 18:02:17 PST
(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?
Comment 19 Adam Barth 2011-02-01 18:18:05 PST
Created attachment 80870 [details]
Might actually be a reasonable approach
Comment 20 Adam Barth 2011-02-01 19:47:34 PST
> 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.
Comment 21 Adam Barth 2011-02-01 20:44:56 PST
Created attachment 80881 [details]
Patch
Comment 22 Eric Seidel (no email) 2011-02-03 17:09:02 PST
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.
Comment 23 Eric Seidel (no email) 2011-02-03 17:10:20 PST
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.
Comment 24 John Abd-El-Malek 2011-02-04 09:06:46 PST
(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?
Comment 25 Adam Barth 2011-02-04 09:55:57 PST
> 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.
Comment 26 Adam Barth 2011-02-04 11:27:28 PST
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.
Comment 27 Eric Seidel (no email) 2011-02-04 11:49:59 PST
Beth recently worked on the post-layout-tasks timer:
https://bugs.webkit.org/show_bug.cgi?id=44828
Comment 28 Eric Seidel (no email) 2011-02-04 12:03:49 PST
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.
Comment 29 Adam Barth 2011-02-04 12:20:41 PST
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.
Comment 30 Eric Seidel (no email) 2011-02-04 13:20:32 PST
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.
Comment 31 Adam Barth 2011-02-04 15:06:02 PST
Created attachment 81297 [details]
test in progress
Comment 32 Adam Barth 2011-02-04 15:34:03 PST
Created attachment 81306 [details]
Patch
Comment 33 Eric Seidel (no email) 2011-02-04 15:34:48 PST
Comment on attachment 81306 [details]
Patch

Thanks.
Comment 34 WebKit Review Bot 2011-02-04 15:36:30 PST
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.
Comment 35 John Abd-El-Malek 2011-02-04 16:19:49 PST
thanks Adam!
Comment 36 Adam Barth 2011-02-04 16:29:53 PST
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.
Comment 37 WebKit Commit Bot 2011-02-04 16:50:17 PST
Comment on attachment 81306 [details]
Patch

Clearing flags on attachment: 81306

Committed r77706: <http://trac.webkit.org/changeset/77706>
Comment 38 WebKit Commit Bot 2011-02-04 16:50:24 PST
All reviewed patches have been landed.  Closing bug.
Comment 39 WebKit Review Bot 2011-02-04 18:32:45 PST
http://trac.webkit.org/changeset/77706 might have broken GTK Linux 64-bit Debug