Summary: | REGRESSION (r102983): ClicktoFlash drawing of old style youtube embeds missing until resize | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kevin M. Dean <kevin> | ||||||||
Component: | Layout and Rendering | Assignee: | Andy Estes <aestes> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, aestes, ap, eric, marc.hoyois | ||||||||
Priority: | P2 | Keywords: | HasReduction, InRadar, Regression | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Mac (Intel) | ||||||||||
OS: | OS X 10.7 | ||||||||||
URL: | http://forum.dvdtalk.com/11092085-post1.html | ||||||||||
Bug Depends on: | 74367 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Kevin M. Dean
2012-01-26 17:40:58 PST
Created attachment 124228 [details]
Screenshot of memory jump
The embed code causing the problem has the form <object> <params …> <embed src="..." type="..." /> </object> The difference with previous versions of WebKit is that the <embed> is ignored if the beforeload event fired by the <object> is handled. ClickToFlash will analyze the beforeload event fired by the <object> and simply return without doing anything, since the <object> will do nothing. But then the page must be redrawn for Safari to take into account the <embed>. So it seems that this is not CTF-specific at all and that there is a bug somewhere in WebKit's object-loading mechanism. This is a result of <http://trac.webkit.org/changeset/102983>. Created attachment 124649 [details]
Minimal HTML file reproducing the bug (disable extensions before checking!)
Added a minimal test case. The problem arises when asking WebKit for the computed width of the object element:
document.addEventListener("beforeload", function(event) {
var w = event.target.offsetWidth; // causes the problem
return;
}, true);
The problem would seem to be that r102983 changed FrameView to only call updateWidget() if needsWidgetUpdate() is true. The problem is that we try to update a widget two times: once for the CreateOnlyNonNetscapePlugins case and again for the CreateAnyWidgetType. The CreateOnlyNonNetscapePlugins phase sets m_needsWidgetUpdate to false, so the CreateAnyWidgetType phase does no work. Created attachment 125982 [details]
Patch
Comment on attachment 125982 [details]
Patch
OK. This is fine. I plan to continue work on further unification of updateWidget code eventually. The test is by-far the most important part of this change. If you're confident in the test, I'm confident in the change.
(In reply to comment #8) > (From update of attachment 125982 [details]) > OK. This is fine. I plan to continue work on further unification of updateWidget code eventually. The test is by-far the most important part of this change. If you're confident in the test, I'm confident in the change. Thanks Eric. I assumed you were in the process of unifying these methods, so I just wanted to put in a short-term fix. Comment on attachment 125982 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125982&action=review > Source/WebCore/html/HTMLPlugInElement.cpp:117 > + // and syncrhonous layout can be initiated in a beforeload event handler! Typo: syncrhonous In that case, won't this function be buggy because it unconditionally sets m_inBeforeLoadEventHandler to false at the end? Do we need to use a counter here? (In reply to comment #10) > (From update of attachment 125982 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125982&action=review > > > Source/WebCore/html/HTMLPlugInElement.cpp:117 > > + // and syncrhonous layout can be initiated in a beforeload event handler! > > Typo: syncrhonous > > In that case, won't this function be buggy because it unconditionally sets m_inBeforeLoadEventHandler to false at the end? Do we need to use a counter here? Yeah, this assertion has been in a tree for a while. I should find if there is a bug already filed about it and file one if not. As far as this patch goes, I think it still makes sense to land it with the assertion removed. It won't change behavior in release builds and allows us to increase our test coverage for this very recent regression. "Yeah, this assertion *failure*..." Fair enough. Would you be willing to file a bug about the re-entrancy problem in that function? (In reply to comment #13) > Fair enough. Would you be willing to file a bug about the re-entrancy problem in that function? Looks like I filed this bug back in October then forgot about it! https://bugs.webkit.org/show_bug.cgi?id=71264 Committed r107118: <http://trac.webkit.org/changeset/107118> |