Bug 86543 – ImageDocuments erroneously trigger beforeload events for the main resource

• Component: Page Loading
• Status: RESOLVED FIXED

[Vicki Pfau]

When an ImageDocument is loading, it creates an <img> tag, which triggers a beforeload event. However, the event corresponds to the main resource, and can cause a crash if the event is handled and preventDefault() is called.

<rdar://problem/11309013>

[Vicki Pfau]

Created attachment 142094 [details]
Patch

[Vicki Pfau]

Committed r117185: <http://trac.webkit.org/changeset/117185>

[Alexey Proskuryakov]

Why is it safe to dispatch beforeload on image documents? This appears to be as dangerous as elsewhere, because a handler can do evil things.

[Brady Eidson]

(In reply to comment #3)
> Why is it safe to dispatch beforeload on image documents? This appears to be as dangerous as elsewhere, because a handler can do evil things.

I'm a little confused about your question.  I don't think anyone here said it is safe to dispatch beforeload on image documents.

But the bug here was really that we were dispatching beforeLoad for an <img> element inside the image document... but in reality the resource was already being loaded; It was the main resource for the document.

Logically - since we don't do beforeLoad for main resources - the resource for an image document shouldn't be any different, and especially not by representing it as an <img> element which is an implementation detail of image documents.

[Vicki Pfau]

I'm not entirely sure why this happens, but by calling dispatchPendingBeforeLoadEvent() in that branch actually prevents the beforeload event from reaching any handlers. It also triggers logic essential to making images show up in the first place. If the second branch is taken, it triggers the beforeload event after handlers get installed, thus the source of the bug. Skipping both branches misses the logic essential to making the images show up. From an objective standpoint, this means that some code is in the wrong place and needs to be refactored. However, I've been unsuccessful at finding where the aforementioned logic takes place, as it's nowhere obvious.

I've filed bug 86658 as a followup.

[Brady Eidson]

(In reply to comment #5)
> I'm not entirely sure why this happens, but by calling dispatchPendingBeforeLoadEvent() in that branch actually prevents the beforeload event from reaching any handlers. It also triggers logic essential to making images show up in the first place. If the second branch is taken, it triggers the beforeload event after handlers get installed, thus the source of the bug. Skipping both branches misses the logic essential to making the images show up. From an objective standpoint, this means that some code is in the wrong place and needs to be refactored. However, I've been unsuccessful at finding where the aforementioned logic takes place, as it's nowhere obvious.
> 
> I've filed bug 86658 as a followup.

I assumed that the very first check in that method - if (!m_hasPendingBeforeLoadEvent) - failed and it bailed out.

But if calling that method actually does other stuff relevant to the image showing up, then my assumption can't possibly have been the case.

I strongly urge you to explore this soon.  Now both Alexey and I are confused.

[Alexey Proskuryakov]

> I'm a little confused about your question.  I don't think anyone here said it is safe to dispatch beforeload on image documents.

That's what the new code says, in a very direct manner. Per Jeffrey's answer, the code lies about what it does, however we don't appear to have complete understanding.

> I'm not entirely sure why this happens, but by calling dispatchPendingBeforeLoadEvent() in that branch actually prevents the beforeload event from reaching any handlers.

Perhaps it fires the event earlier, so handlers installed on img element don't have a chance to run? But handlers installed on ancestors would fire, and in a manner that's even worse than before?

[Vicki Pfau]

(In reply to comment #7) 
> > I'm not entirely sure why this happens, but by calling dispatchPendingBeforeLoadEvent() in that branch actually prevents the beforeload event from reaching any handlers.
> 
> Perhaps it fires the event earlier, so handlers installed on img element don't have a chance to run? But handlers installed on ancestors would fire, and in a manner that's even worse than before?

The event doesn't bubble, so ancestors never see it.

I've stepped through the code somewhat thoroughly and it looks like it bails out without actually doing anything noteworthy, but if I skip that code entirely, then img tags don't work. So it looks like some constructor somewhere must be doing the relevant work, I'm just not sure which one.

[Alexey Proskuryakov]

> representing it as an <img> element which is an implementation detail of image documents

I've been under the impression that HTML5 specifies this, however I cannot find any proof now.

[Alexey Proskuryakov]

> The event doesn't bubble, so ancestors never see it.

That's not what I'm seeing in my testing - the below snippet logs both "bl" and "bl2".

<body>
<script>
document.body.addEventListener("beforeload", function() { console.log('bl') }, true);
document.addEventListener("beforeload", function() { console.log('bl2') }, true);
</script>
<img src="aaa">
<img src="bbb">

[Alexey Proskuryakov]

Well, that's a capturing listener, so it doesn't actually contradict what you said about bubbling, but it still shows that listeners are invoked on ancestors.

[Ian 'Hixie' Hickson]

(In reply to comment #11)
> Well, that's a capturing listener, so it doesn't actually contradict what you said about bubbling, but it still shows that listeners are invoked on ancestors.

In the capture phase, events are seen by all ancestors. In the bubbling phase, they're only seen if they bubble.

(In reply to comment #9)
> > representing it as an <img> element which is an implementation detail of image documents
> 
> I've been under the impression that HTML5 specifies this, however I cannot find any proof now.

See the navigation section.