Bug 44575 - beforeload event fired twice on <object>s
Summary: beforeload event fired twice on <object>s
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.6
: P2 Normal
Assignee: Nobody
Keywords: InRadar
: 49123 (view as bug list)
Depends on:
Reported: 2010-08-24 19:21 PDT by Sidney San Martín
Modified: 2011-09-09 15:57 PDT (History)
11 users (show)

See Also:

Test case demonstrating duplicate beforeload events (312 bytes, text/html)
2010-08-24 19:21 PDT, Sidney San Martín
no flags Details
Stacktrace when dispatchBeforeLoadEvent is called during window resizes (4.50 KB, text/plain)
2010-08-25 08:46 PDT, Mihai Parparita
no flags Details
Test case demonstrating what I was talking about in comment #11 (411 bytes, text/html)
2011-09-09 15:57 PDT, Marc Hoyois
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sidney San Martín 2010-08-24 19:21:20 PDT
Created attachment 65360 [details]
Test case demonstrating duplicate beforeload events

I'm hoping that I'm missing something about the beforeload event, and that this isn't a bug.

The attached testcase increments a counter each time beforeload fires, and logs the event's target element to the console.

Expected results:
beforeload fires once on the <object>

Actual results:
beforeload appears to fire twice on the <object>
Comment 1 Eric Seidel (no email) 2010-08-24 21:34:19 PDT
Is this a recent change?
Comment 2 Sidney San Martín 2010-08-24 21:38:42 PDT
(In reply to comment #1)
> Is this a recent change?

I don't think so, but I just noticed it. I can reproduce in Safari 5.0.1 and Chrome 5.0.375.127.
Comment 3 Mihai Parparita 2010-08-25 08:46:27 PDT
Created attachment 65422 [details]
Stacktrace when dispatchBeforeLoadEvent is called during window resizes

This is because we end up dispatching beforeload for every single layout (and your logging triggers a layout, as opening the inspector console). The easiest way to see this is to resize the window while showing the testcase, the beforeload count keeps going up.

Attached is the stack trace that shows how we get to dispatching the beforeload event during a resize. This is technically correct, since we do end up calling requestObject shortly after.
Comment 4 Eric Seidel (no email) 2010-08-25 09:47:06 PDT
Hyatt added beforeload.  Jamesr has talked about moving object/embed loading out of the renderers.  Both may be interested in this bug.
Comment 5 James Robinson 2010-08-25 13:14:29 PDT
We probably shouldn't fire beforeload at all if the <object>'s URL is empty, since we'll use the fallback content regardless in that case.  That would at least paper over this issue.
Comment 6 Marc Hoyois 2010-09-16 14:25:24 PDT
I'm developing the ClickToFlash extension for Safari and the fact that some <object>s fire two beforeload events in very quick succession is something I noticed early on. It's relatively common, in fact. It's not random in that a given object seems to always fire it once or always twice, but I have no idea what characteristic of the object is determinant. Here's a real-life example of an embedded Windows Media video that always fires it twice:


Also, I noticed that in the attached example, as soon as you add some attributes to the object (eg. add type="blah"), resizing does not make it fire more events anymore. Doing that increases the count by 2 instead of 1. However, if you add something that actually launches a plugin, like type="application/x-shockwave-flash", the count only increases by 1.
Comment 7 Alexey Proskuryakov 2010-09-16 14:52:18 PDT
Comment 8 Alexey Proskuryakov 2010-10-11 15:14:48 PDT
I think that this can be fixed by enforcing a comment in HTMLObjectElement::updateWidget():

    // FIXME: We should ASSERT(needsWidgetUpdate()), but currently
    // FrameView::updateWidget() calls updateWidget(false) without checking if
    // the widget actually needs updating!

Adding an early return in this case sounds like a right thing to do.

Another approach that looks like a right thing to do is removing HTMLPlugInImageElement::updateWidgetIfNecessary() and all associated machinery! All it does is call updateStyleIfNeeded(), and then updateWidget(true), which is almost a no-op for NPAPI plug-ins. And I don't see why WebKit plug-ins should be different here.

So, updateWidgetIfNecessary ends up calling updateStyleIfNeeded(), and then setNeedsWidgetUpdate(false), and that's it - which makes no sense. When I made this change, one regression test broke, but another one progressed to match Firefox.
Comment 9 Eric Seidel (no email) 2010-10-11 21:27:35 PDT
updateWidgetIfNecessary is vestigial.  It used to be much more complicated, but now it should just be removed it possible.   The whole "updateWidget" nonsense should be.
Comment 10 Andy Estes 2010-11-08 17:45:51 PST
*** Bug 49123 has been marked as a duplicate of this bug. ***
Comment 11 Marc Hoyois 2011-09-07 16:03:13 PDT
This may be related to comment #8: trying to get computed styles of event.target within the handler can cause the handler to pause while a new beforeload event is fired and handled. This seems to happen consistently if plugins are disabled. I'd like to point out that it's essential for several content-blocking extensions out there that computed styles be accessible within the beforeload handler, so please don't remove this possibility. The 2nd beforeload shouldn't be there, though (but it's easy to ignore in Javascript: just set a property on event.target before getting styles and remove it after).

Another completely different cause of duplicate beforeload events concerns native image types in <object> elements. For example if the specified type is image/jpeg, Safari tries to load the resource directly, but if that fails it checks the actual Content-Type and fires a beforeload again. In this case the second event is fired some time after the first has been handled, and so it's impossible to tell for sure that the second event is a duplicate. Instead of reusing the same <object> after checking the Content-Type, it would be more consistent if WebKit created a new subframe like it does in the case when type is not set and extension does not match a plugin, and fire the 2nd beforeload there.

These two phenomena can even combine so that <object> elements can fire up to 3 beforeload events.

It seems to me the main problem is that the beforeload event is not fired in the correct place: updateWidget is called in cases where no beforeload should be fired. Conceptually the algorithm should be

1. Gather parameters from the DOM. If no source and no type, abort and use fallback. (Only source and type are needed here, but since we must look for them in <params> for legacy reasons, we might as well gather all parameters now. It would even be useful to set them as a DOMStringMap property of the beforeload event.)
2. Fire beforeload. If defaultPrevented, abort and use fallback.
3. Run the loading procedures.

Start again whenever the data/src or type attributes are changed. Currently beforeload is fired in the middle of 3...
Comment 12 Marc Hoyois 2011-09-09 15:21:57 PDT
It seems that beforeload events are always fired twice (at least on <embeds>) when plugins are disabled in Safari's preferences. I assumed that was the same as having no plugins installed, but it's not. Evaluation of things like event.target.offsetWidth or event.target.parentNode.toString() if the parent is an <object> create a 2nd beforeload dispatch on event.target within the handler, but otherwise a 2nd beforeload is still fired after the first has been handled.

Anyway I cannot reproduce the first example in my comment #11 if plugins are enabled, so this is not quite a real-life example, but still a valid test case to keep in mind to make sure beforeload dispatch is logically implemented.
Comment 13 Marc Hoyois 2011-09-09 15:57:11 PDT
Created attachment 106932 [details]
Test case demonstrating what I was talking about in comment #11

Scratch that, my first example in comment #11 can also happen with plugins enabled. I attach a test case with the <object><embed> combination. The console output is

1 before accessing parentNode
2 before accessing parentNode
2 after accessing parentNode
1 after accessing parentNode

which shows that accessing the <object> element while handling the beforeload of the <embed> element dispatches a new beforeload on <embed>. You can also have the <embed> element alone and evaluate event.target.offsetWidth or getComputedStyle(event.target, null): same result.