Bug 44575 – beforeload event fired twice on <object>s

• Component: WebCore Misc.
• Status: NEW

[Sidney San Martín]

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>

[Eric Seidel (no email)]

Is this a recent change?

[Sidney San Martín]

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

[Mihai Parparita]

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.

[Eric Seidel (no email)]

Hyatt added beforeload.  Jamesr has talked about moving object/embed loading out of the renderers.  Both may be interested in this bug.

[James Robinson]

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.

[Marc Hoyois]

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:

http://www.mediacollege.com/video/format/windows-media/streaming/embed.html

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.

[Alexey Proskuryakov]

<rdar://problem/8441094>

[Alexey Proskuryakov]

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.

[Eric Seidel (no email)]

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.

[Andy Estes]

*** Bug 49123 has been marked as a duplicate of this bug. ***

[Marc Hoyois]

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

[Marc Hoyois]

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.

[Marc Hoyois]

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.