Bug 86543

Summary: ImageDocuments erroneously trigger beforeload events for the main resource
Product: WebKit Reporter: Vicki Pfau <jeffrey+webkit>
Component: Page LoadingAssignee: Vicki Pfau <jeffrey+webkit>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, ap, beidson, ian, japhet, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch beidson: review+

Description Vicki Pfau 2012-05-15 16:21:02 PDT
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>
Comment 1 Vicki Pfau 2012-05-15 16:25:59 PDT
Created attachment 142094 [details]
Patch
Comment 2 Vicki Pfau 2012-05-15 16:49:15 PDT
Committed r117185: <http://trac.webkit.org/changeset/117185>
Comment 3 Alexey Proskuryakov 2012-05-16 10:53:13 PDT
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.
Comment 4 Brady Eidson 2012-05-16 11:13:55 PDT
(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.
Comment 5 Vicki Pfau 2012-05-16 11:17:10 PDT
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.
Comment 6 Brady Eidson 2012-05-16 11:22:15 PDT
(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.
Comment 7 Alexey Proskuryakov 2012-05-16 11:26:27 PDT
> 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?
Comment 8 Vicki Pfau 2012-05-16 11:30:40 PDT
(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.
Comment 9 Alexey Proskuryakov 2012-05-16 11:30:58 PDT
> 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.
Comment 10 Alexey Proskuryakov 2012-05-16 11:34:29 PDT
> 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">
Comment 11 Alexey Proskuryakov 2012-05-16 11:35:50 PDT
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.
Comment 12 Ian 'Hixie' Hickson 2012-05-19 22:14:22 PDT
(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.