Bug 40915 – beforeload event is not sent for first image when @src is specified before @onbeforeload

• Status: ASSIGNED
• Component: Page Loading

[Adam Roben (:aroben)]

Created attachment 59248 [details]
test case

To reproduce:

1. Load the attached test case

The beforeload event only fires for the second and third images (as evidenced by only "2" and "3" being printed).

[Adam Roben (:aroben)]

<rdar://problem/7630837>

[Adam Roben (:aroben)]

It looks like the load for the first <img> is getting kicked off before we set up the beforeload event listener. Element::setAttributeMap calls attributeChanged for each added attribute. The src attribute is coming before the onbeforeload attribute, so when the load begins we haven't noticed the onbeforeload attribute yet.

[Adam Roben (:aroben)]

We bail out of sending the event in ContainerNode::dispatchBeforeLoadEvent:

    if (!document()->hasListenerType(Document::BEFORELOAD_LISTENER))
        return true;

[Adam Roben (:aroben)]

Changing the order of the attributes in the test case to put the onbeforeload attribute before the src attribute fixes the bug.

[Adam Roben (:aroben)]

Perhaps this is expected behavior?

[Alexey Proskuryakov]

I don't think order of attributes is ever expected to have any effect on behavior.

[Alexey Proskuryakov]

Maybe this can be fixed in more or less the same way as bug 40742 was.

[Dave Hyatt]

In order to fix this it will probably be necessary to move image src updating in the parsing case to finishParsingChildren instead of doing it right when the attribute changes.

[Darin Adler]

(In reply to comment #8)
> In order to fix this it will probably be necessary to move image src updating in the parsing case to finishParsingChildren instead of doing it right when the attribute changes.

We can't use isFinishedParsingChildren() in parseMappedAttribute because we don’t clear that flag until we are done with the attributes and start in on the children. So we might need one of those annoying "created by parser" flags that are used in classes like HTMLScriptElement and HTMLStyleElement.

I think finishParsingChildren is OK, but a bit too late. We could probably do it in beginParsingChildren if we wanted.

Attributes are all set at once. It's only the calls to attributeChanged that are done sequentially in setAttributeMap. So, ironically, the onbeforeload attribute has already been set, it's just that attributeChanged has not been called on it yet. I don’t see any obvious way to know we are inside setAttributeMap without using a createdByParser flag.

[Andy Estes]

In ImageLoader::updateFromElement(), we could just change:

    if (!m_element->document()->hasListenerType(Document::BEFORELOAD_LISTENER))
        dispatchPendingBeforeLoadEvent();
    else
        beforeLoadEventSender().dispatchEventSoon(this);

to:

    beforeLoadEventSender().dispatchEventSoon(this);

Asynchronously firing beforeload gives HTMLImageElement a chance to parse all the attributes, and it doesn't delay processing any more than it already is when @onbeforeload is before @src.  OTOH, there might be a downside here that I'm not comprehending.

[Darin Adler]

(In reply to comment #10)
> In ImageLoader::updateFromElement(), we could just change:
> 
>     if (!m_element->document()->hasListenerType(Document::BEFORELOAD_LISTENER))
>         dispatchPendingBeforeLoadEvent();
>     else
>         beforeLoadEventSender().dispatchEventSoon(this);
> 
> to:
> 
>     beforeLoadEventSender().dispatchEventSoon(this);
> 
> Asynchronously firing beforeload gives HTMLImageElement a chance to parse all the attributes, and it doesn't delay processing any more than it already is when @onbeforeload is before @src.  OTOH, there might be a downside here that I'm not comprehending.

I think a downside is this would make all image loading slower, since we’d always wait one run loop iteration. Not my area of expertise, though, so I’m not sure.

[Dave Hyatt]

Something I should have clarified... this is not an urgent bug to fix.... no extension is going to hook up beforeload this way.  Because it injects via user script, you're going to see event listeners hooked up in JS using the addEventListener API.  You're just not going to see this usage typically.

Therefore this is the least urgent of the current crop of beforeload bugs.

[Dave Hyatt]

And yeah what Darin said is exactly right.  You dispatch the event synchronously because you know nobody is listening.  This allows you to propagate the image to the render tree immediately, which is especially important if the image has already loaded.

If you just always make the renderer update async, then you're going to hurt performance.

[Darin Adler]

Note that due to the nature of the bug, this will only happen for the first element in a document that has a beforeload event handler.