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
Created attachment 124228 [details] Screenshot of memory jump
<rdar://problem/10767026>
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>