86543 2012-05-15 16:21:02 -0700 ImageDocuments erroneously trigger beforeload events for the main resource 2012-05-19 22:14:22 -0700 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 jeffrey+webkit jeffrey+webkit aestes ap beidson ian japhet webkit.review.bot oldest_to_newest 624711 0 jeffrey+webkit 2012-05-15 16:21:02 -0700 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> 624717 1 142094 jeffrey+webkit 2012-05-15 16:25:59 -0700 Created attachment 142094 Patch 624733 2 jeffrey+webkit 2012-05-15 16:49:15 -0700 Committed r117185: <http://trac.webkit.org/changeset/117185> 625634 3 ap 2012-05-16 10:53:13 -0700 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. 625652 4 beidson 2012-05-16 11:13:55 -0700 (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. 625659 5 jeffrey+webkit 2012-05-16 11:17:10 -0700 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. 625667 6 beidson 2012-05-16 11:22:15 -0700 (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. 625671 7 ap 2012-05-16 11:26:27 -0700 > 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? 625678 8 jeffrey+webkit 2012-05-16 11:30:40 -0700 (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. 625679 9 ap 2012-05-16 11:30:58 -0700 > 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. 625685 10 ap 2012-05-16 11:34:29 -0700 > 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"> 625687 11 ap 2012-05-16 11:35:50 -0700 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. 628546 12 ian 2012-05-19 22:14:22 -0700 (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. 142094 2012-05-15 16:25:59 -0700 2012-05-15 16:29:55 -0700 Patch bug-86543-20120515162602.patch text/plain 1575 jeffrey+webkit U3VidmVyc2lvbiBSZXZpc2lvbjogMTE2Nzk2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggN2FmMzU3ODUzYTRiYjhi ZTQ0YzZiNjNlN2M1YzI3Y2ExNjVhODEzZC4uYTdmN2Q3NjYxN2E3YmY0MjY0OWQ5NTAxZTNkN2Uy ZDc2NWM4MzI5NCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDEyLTA1LTE1ICBKZWZm cmV5IFBmYXUgIDxqcGZhdUBhcHBsZS5jb20+CisKKyAgICAgICAgSW1hZ2VEb2N1bWVudHMgZXJy b25lb3VzbHkgdHJpZ2dlciBiZWZvcmVsb2FkIGV2ZW50cyBmb3IgdGhlIG1haW4gcmVzb3VyY2UK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTg2NTQzCisK KyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm8gbmV3IHRl c3RzOyB0ZXN0aW5nIGZyYW1ld29yayBkb2Vzbid0IGFsbG93IGZvciB0ZXN0aW5nIEltYWdlRG9j dW1lbnRzIHdpdGggaW5qZWN0ZWQgSmF2YVNjcmlwdC4KKworICAgICAgICAqIGxvYWRlci9JbWFn ZUxvYWRlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpJbWFnZUxvYWRlcjo6dXBkYXRlRnJvbUVs ZW1lbnQpOgorCiAyMDEyLTA1LTExICBBbGV4YW5kcnUgQ2hpY3VsaXRhICA8YWNoaWN1QGFkb2Jl LmNvbT4KIAogICAgICAgICBbQ1NTIFNoYWRlcnNdIE1ha2UgQ1NTIFNoYWRlcnMgcmVuZGVyIHRv IHRleHR1cmUgZnJhbWVidWZmZXJzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9sb2FkZXIv SW1hZ2VMb2FkZXIuY3BwIGIvU291cmNlL1dlYkNvcmUvbG9hZGVyL0ltYWdlTG9hZGVyLmNwcApp bmRleCBjNzFkMzFhM2M1MzE0MGFlMWI1Mzc2ZjY5OGExMDc4YzE5YjA4NGRhLi5kNGFmNmVmODZj MDE4YmU0OTIzZmNhZmU2NDA3NzU1MjQzNmU5NWIzIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9sb2FkZXIvSW1hZ2VMb2FkZXIuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9JbWFn ZUxvYWRlci5jcHAKQEAgLTE5OSw3ICsxOTksNyBAQCB2b2lkIEltYWdlTG9hZGVyOjp1cGRhdGVG cm9tRWxlbWVudCgpCiAgICAgICAgIG1faW1hZ2VDb21wbGV0ZSA9ICFuZXdJbWFnZTsKIAogICAg ICAgICBpZiAobmV3SW1hZ2UpIHsKLSAgICAgICAgICAgIGlmICghbV9lbGVtZW50LT5kb2N1bWVu dCgpLT5oYXNMaXN0ZW5lclR5cGUoRG9jdW1lbnQ6OkJFRk9SRUxPQURfTElTVEVORVIpKQorICAg ICAgICAgICAgaWYgKCFtX2VsZW1lbnQtPmRvY3VtZW50KCktPmhhc0xpc3RlbmVyVHlwZShEb2N1 bWVudDo6QkVGT1JFTE9BRF9MSVNURU5FUikgfHwgbV9lbGVtZW50LT5kb2N1bWVudCgpLT5pc0lt YWdlRG9jdW1lbnQoKSkKICAgICAgICAgICAgICAgICBkaXNwYXRjaFBlbmRpbmdCZWZvcmVMb2Fk RXZlbnQoKTsKICAgICAgICAgICAgIGVsc2UKICAgICAgICAgICAgICAgICBiZWZvcmVMb2FkRXZl bnRTZW5kZXIoKS5kaXNwYXRjaEV2ZW50U29vbih0aGlzKTsK