Bug 47550

Summary: For WebKit plug-ins, beforeload can be called recursively (esp. with AdBlock style extensions)
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: Plug-insAssignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal CC: mitz, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 74340    
Attachments:
Description Flags
test case (change MIME type to a plug-in you have installed)
none
proposed patch simon.fraser: review+

Description Alexey Proskuryakov 2010-10-12 11:54:47 PDT
Created attachment 70550 [details]
test case (change MIME type to a plug-in you have installed)

This happens with AdBlock extension and any WebKit-style plug-in:
1. Add a plug-in element to a document.
2. Access any property, e.g. myPlugin.myProperty. This makes the plug-in load, since myProperty can be defined in the plug-in. As the plug-in is loaded, a beforeload event is dispatched.
3. In beforeload handler, access e.g. myPlugin.nodeName. Since the plug-in hasn't loaded yet, we go back into HTMLObjectElement::updateWidget(), and dispatch beforeload again.

Two of the ways updateWidget is triggered are style resolution and layout. The interaction of these result in one recursive call, and also Widget object is created twice. So, the plug-in is stopped, and malfunctions in the future.

<rdar://problem/8353386>
Comment 1 Alexey Proskuryakov 2010-10-12 12:30:33 PDT
Created attachment 70554 [details]
proposed patch
Comment 2 Simon Fraser (smfr) 2010-10-12 13:12:48 PDT
Comment on attachment 70554 [details]
proposed patch

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

> WebCore/html/HTMLPlugInElement.cpp:108
> +    if (m_inBeforeLoadEventHandler) {
> +        // The plug-in hasn't loaded yet, and it makes no sense to try to load if beforeload handler happened to touch the plug-in element.
> +        // That would recursively call beforeload for the same element.
> +        return false;
> +    }

This should return 0, not return false.
Comment 3 Alexey Proskuryakov 2010-10-12 13:29:47 PDT
Committed <http://trac.webkit.org/changeset/69596>.