Bug 77167 – REGRESSION (r102983): ClicktoFlash drawing of old style youtube embeds missing until resize

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Kevin M. Dean]

First the ClicktoFlash extension needs to be installed.

Whether I'm set to allow flash from www.youtube.com as a source or not in ClicktoFlash, the provided page will load with empty white space where the youtube video or ClicktoFlash placeholder should be. Then if I resize my window, the missing youtube embed or ClicktoFlash placeholder will then appear. Seems a relayout causes the OBJECT/EMBED or placeholder to now render.

Note that it works fine without ClicktoFlash, but also used to work with ClicktoFlash in r102809 and earlier. There was a nightly (r102930) between my range that I wasn't able to fully confirm because the page would crash 3 times in a row and then show the video embed with that version.

Here's that crash thread in case it has bearing even though the current nightlies no longer crash.

(Oddly while writing this report, Webkit became unresponsive and I noticed my Web Process was using over 9 GB of RAM forcing me to take a quick screenshot for retyping and then force quit Webkit... maybe loading and reloading that page also causes a huge memory leak as well. I'll need to see if I can reproduce it)

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebKit2             	0x000000010b403896 CoreIPC::Connection::dispatchMessages() + 92
1   com.apple.WebKit2             	0x000000010b41cba7 RunLoop::performWork() + 111
2   com.apple.WebKit2             	0x000000010b41cfb7 RunLoop::performWork(void*) + 75
3   com.apple.CoreFoundation      	0x00007fff8891eb51 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
4   com.apple.CoreFoundation      	0x00007fff8891e47c __CFRunLoopDoSources0 + 444
5   com.apple.CoreFoundation      	0x00007fff889451a9 __CFRunLoopRun + 905
6   com.apple.CoreFoundation      	0x00007fff88944ae6 CFRunLoopRunSpecific + 230
7   com.apple.HIToolbox           	0x00007fff8cac63d3 RunCurrentEventLoopInMode + 277
8   com.apple.HIToolbox           	0x00007fff8cacd63d ReceiveNextEventCommon + 355
9   com.apple.HIToolbox           	0x00007fff8cacd4ca BlockUntilNextEventMatchingListInMode + 62
10  com.apple.AppKit              	0x00007fff84c553f1 _DPSNextEvent + 659
11  com.apple.AppKit              	0x00007fff84c54cf5 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 135
12  com.apple.AppKit              	0x00007fff84c5162d -[NSApplication run] + 470
13  com.apple.WebKit2             	0x000000010b41d177 RunLoop::run() + 67
14  com.apple.WebKit2             	0x000000010b4650c4 WebKit::WebProcessMain(WebKit::CommandLine const&) + 678
15  com.apple.WebKit2             	0x000000010b436a41 WebKitMain + 285
16  com.apple.WebProcess          	0x000000010b29ae5f main + 219
17  com.apple.WebProcess          	0x000000010b29ad7c start + 52

[Kevin M. Dean]

Created attachment 124228 [details]
Screenshot of memory jump

[Alexey Proskuryakov]

<rdar://problem/10767026>

[Marc Hoyois]

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.

[Andy Estes]

This is a result of <http://trac.webkit.org/changeset/102983>.

[Marc Hoyois]

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);

[Andy Estes]

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.

[Andy Estes]

Created attachment 125982 [details]
Patch

[Eric Seidel (no email)]

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.

[Andy Estes]

(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.

[Adam Barth]

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?

[Andy Estes]

(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.

[Andy Estes]

"Yeah, this assertion *failure*..."

[Adam Barth]

Fair enough.  Would you be willing to file a bug about the re-entrancy problem in that function?

[Andy Estes]

(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

[Andy Estes]

Committed r107118: <http://trac.webkit.org/changeset/107118>