44575 2010-08-24 19:21:20 -0700 beforeload event fired twice on <object>s 2011-09-09 15:57:11 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Mac (Intel) OS X 10.6 NEW InRadar P2 Normal --- 1 s+webkit webkit-unassigned abarth aestes ap aroben eric hyatt jamesr marc.hoyois mitz simon.fraser webkit-bug-importer oldest_to_newest 268973 0 65360 s+webkit 2010-08-24 19:21:20 -0700 Created attachment 65360 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> 268999 1 eric 2010-08-24 21:34:19 -0700 Is this a recent change? 269000 2 s+webkit 2010-08-24 21:38:42 -0700 (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. 269261 3 65422 mihaip 2010-08-25 08:46:27 -0700 Created attachment 65422 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. 269295 4 eric 2010-08-25 09:47:06 -0700 Hyatt added beforeload. Jamesr has talked about moving object/embed loading out of the renderers. Both may be interested in this bug. 269455 5 jamesr 2010-08-25 13:14:29 -0700 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. 280358 6 marc.hoyois 2010-09-16 14:25:24 -0700 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. 280375 7 ap 2010-09-16 14:52:18 -0700 <rdar://problem/8441094> 292694 8 ap 2010-10-11 15:14:48 -0700 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. 292857 9 eric 2010-10-11 21:27:35 -0700 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. 306520 10 aestes 2010-11-08 17:45:51 -0800 *** Bug 49123 has been marked as a duplicate of this bug. *** 463736 11 marc.hoyois 2011-09-07 16:03:13 -0700 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... 465055 12 marc.hoyois 2011-09-09 15:21:57 -0700 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. 465074 13 106932 marc.hoyois 2011-09-09 15:57:11 -0700 Created attachment 106932 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. 65360 2010-08-24 19:21:20 -0700 2010-08-24 19:21:20 -0700 Test case demonstrating duplicate beforeload events webkit_beforeload_multiple_ssm.html text/html 312 s+webkit PCFET0NUWVBFIGh0bWw+CjxodG1sPgo8aGVhZD4KPGJvZHk+CjxwPjxjb2RlPmJlZm9yZWxvYWQ8 L2NvZGU+IGNvdW50OiA8c3Bhbj4wPC9zcGFuPjwvcD4KPHNjcmlwdD4KdmFyIGhpdENvdW50ID0g MCwgaGl0RWxlbWVudCA9IGRvY3VtZW50LmdldEVsZW1lbnRzQnlUYWdOYW1lKCdzcGFuJylbMF07 CmRvY3VtZW50LmFkZEV2ZW50TGlzdGVuZXIoImJlZm9yZWxvYWQiLCBmdW5jdGlvbihlKXsgaGl0 RWxlbWVudC50ZXh0Q29udGVudCA9ICsraGl0Q291bnQ7IGNvbnNvbGUubG9nKGUudGFyZ2V0KSB9 LCB0cnVlKTsKPC9zY3JpcHQ+CjxvYmplY3Q+ 65422 2010-08-25 08:46:27 -0700 2010-08-25 08:46:27 -0700 Stacktrace when dispatchBeforeLoadEvent is called during window resizes dispatchBeforeLoadEvent stack trace.txt text/plain 4612 mihaip IzAgIFdlYkNvcmU6OkNvbnRhaW5lck5vZGU6OmRpc3BhdGNoQmVmb3JlTG9hZEV2ZW50ICh0aGlz PTB4MTA2MzllZTUwLCBzb3VyY2VVUkw9QDB4N2ZmZjVmYmZjY2IwKSBhdCBXZWJLaXQvV2ViQ29y ZS9kb20vQ29udGFpbmVyTm9kZS5jcHA6MTA0NQojMSAgMHgwMDAwMDAwMTAxZGNlMzUyIGluIFdl YkNvcmU6OlJlbmRlckVtYmVkZGVkT2JqZWN0Ojp1cGRhdGVXaWRnZXQgKHRoaXM9MHgxMDYzOTM2 YjgsIG9ubHlDcmVhdGVOb25OZXRzY2FwZVBsdWdpbnM9ZmFsc2UpIGF0IFdlYktpdC9XZWJDb3Jl L3JlbmRlcmluZy9SZW5kZXJFbWJlZGRlZE9iamVjdC5jcHA6Mjk3CiMyICAweDAwMDAwMDAxMDE4 MTFiMWYgaW4gV2ViQ29yZTo6RnJhbWVWaWV3Ojp1cGRhdGVXaWRnZXRzICh0aGlzPTB4MTA2M2Ix ODUwKSBhdCBXZWJLaXQvV2ViQ29yZS9wYWdlL0ZyYW1lVmlldy5jcHA6MTU3NwojMyAgMHgwMDAw MDAwMTAxODExZTUxIGluIFdlYkNvcmU6OkZyYW1lVmlldzo6cGVyZm9ybVBvc3RMYXlvdXRUYXNr cyAodGhpcz0weDEwNjNiMTg1MCkgYXQgV2ViS2l0L1dlYkNvcmUvcGFnZS9GcmFtZVZpZXcuY3Bw OjE2MDcKIzQgIDB4MDAwMDAwMDEwMTgxNDdiYyBpbiBXZWJDb3JlOjpGcmFtZVZpZXc6OmxheW91 dCAodGhpcz0weDEwNjNiMTg1MCwgYWxsb3dTdWJ0cmVlPXRydWUpIGF0IFdlYktpdC9XZWJDb3Jl L3BhZ2UvRnJhbWVWaWV3LmNwcDo4MjYKIzUgIDB4MDAwMDAwMDEwMTgxNGI5NyBpbiBXZWJDb3Jl OjpGcmFtZVZpZXc6OmZvcmNlTGF5b3V0ICh0aGlzPTB4MTA2M2IxODUwLCBhbGxvd1N1YnRyZWU9 dHJ1ZSkgYXQgV2ViS2l0L1dlYkNvcmUvcGFnZS9GcmFtZVZpZXcuY3BwOjIwMzIKIzYgIDB4MDAw MDAwMDEwMGYyZmVmNCBpbiAtW1dlYkhUTUxWaWV3IGxheW91dFRvTWluaW11bVBhZ2VXaWR0aDpo ZWlnaHQ6bWF4aW11bVBhZ2VXaWR0aDphZGp1c3RpbmdWaWV3U2l6ZTpdIChzZWxmPTB4MTA2Mzkw YjMwLCBfY21kPTB4MTAxMDEyY2MzLCBtaW5QYWdlV2lkdGg9MCwgbWluUGFnZUhlaWdodD0wLCBt YXhQYWdlV2lkdGg9MCwgYWRqdXN0Vmlld1NpemU9MCAnXDAnKSBhdCBXZWJLaXQvV2ViS2l0L21h Yy9XZWJWaWV3L1dlYkhUTUxWaWV3Lm1tOjMxNjgKIzcgIDB4MDAwMDAwMDEwMGYyMWRlYiBpbiAt W1dlYkhUTUxWaWV3IGxheW91dF0gKHNlbGY9MHgxMDYzOTBiMzAsIF9jbWQ9MHg3ZmZmODRjNWRm ZTMpIGF0IFdlYktpdC9XZWJLaXQvbWFjL1dlYlZpZXcvV2ViSFRNTFZpZXcubW06MzE4MgojOCAg MHgwMDAwMDAwMTAwZWVhNWIzIGluIC1bV2ViRHluYW1pY1Njcm9sbEJhcnNWaWV3KFdlYkludGVy bmFsKSB1cGRhdGVTY3JvbGxlcnNdIChzZWxmPTB4MTA2Mzc4MjgwLCBfY21kPTB4N2ZmZjg4Nzc1 Y2Y2KSBhdCBXZWJLaXQvV2ViS2l0L21hYy9XZWJWaWV3L1dlYkR5bmFtaWNTY3JvbGxCYXJzVmll dy5tbToyMTcKIzkgIDB4MDAwMDAwMDEwMGVlOWJkOCBpbiAtW1dlYkR5bmFtaWNTY3JvbGxCYXJz VmlldyhXZWJJbnRlcm5hbCkgcmVmbGVjdFNjcm9sbGVkQ2xpcFZpZXc6XSAoc2VsZj0weDEwNjM3 ODI4MCwgX2NtZD0weDdmZmY4NGMzNzU0YywgY2xpcFZpZXc9MHgxMDYzNTE2ZjApIGF0IFdlYktp dC9XZWJLaXQvbWFjL1dlYlZpZXcvV2ViRHluYW1pY1Njcm9sbEJhcnNWaWV3Lm1tOjM0OAojMTAg MHgwMDAwN2ZmZjg0NTQ0ODgxIGluIC1bTlNDbGlwVmlldyBfc2VsZkJvdW5kc0NoYW5nZWRdICgp CiMxMSAweDAwMDA3ZmZmODQ1N2RkMTEgaW4gLVtOU0NsaXBWaWV3IHNldEZyYW1lU2l6ZTpdICgp CiMxMiAweDAwMDA3ZmZmODQ0ZmMxOWEgaW4gLVtOU1ZpZXcgc2V0RnJhbWU6XSAoKQojMTMgMHgw MDAwN2ZmZjg0NTk2MmZhIGluIC1bTlNTY3JvbGxWaWV3IF9hcHBseUNvbnRlbnRBcmVhTGF5b3V0 Ol0gKCkKIzE0IDB4MDAwMDdmZmY4NDU5NWE5YiBpbiAtW05TU2Nyb2xsVmlldyB0aWxlXSAoKQoj MTUgMHgwMDAwMDAwMTAwZWVhMDY1IGluIC1bV2ViRHluYW1pY1Njcm9sbEJhcnNWaWV3KFdlYklu dGVybmFsKSB0aWxlXSAoc2VsZj0weDEwNjM3ODI4MCwgX2NtZD0weDdmZmY4NGMxOGI1OCkgYXQg V2ViS2l0L1dlYktpdC9tYWMvV2ViVmlldy9XZWJEeW5hbWljU2Nyb2xsQmFyc1ZpZXcubW06MTY5 CiMxNiAweDAwMDA3ZmZmODQ1OTUyYzcgaW4gLVtOU1Njcm9sbFZpZXcgX3RpbGVXaXRob3V0UmVj dXJzaW5nXSAoKQojMTcgMHgwMDAwN2ZmZjg0NTk1MjgyIGluIC1bTlNTY3JvbGxWaWV3IF91cGRh dGVdICgpCiMxOCAweDAwMDA3ZmZmODQ0ZmM4M2UgaW4gLVtOU1ZpZXcgc2V0RnJhbWVTaXplOl0g KCkKIzE5IDB4MDAwMDdmZmY4NDU0MWYzMiBpbiAtW05TU2Nyb2xsVmlldyBzZXRGcmFtZVNpemU6 XSAoKQojMjAgMHgwMDAwN2ZmZjg0NGZjMTlhIGluIC1bTlNWaWV3IHNldEZyYW1lOl0gKCkKIzIx IDB4MDAwMDdmZmY4NDU3YzFhNCBpbiAtW05TVmlldyByZXNpemVXaXRoT2xkU3VwZXJ2aWV3U2l6 ZTpdICgpCiMyMiAweDAwMDA3ZmZmODQ1MDI1NTAgaW4gLVtOU1ZpZXcgcmVzaXplU3Vidmlld3NX aXRoT2xkU2l6ZTpdICgpCiMyMyAweDAwMDA3ZmZmODQ0ZmM4M2UgaW4gLVtOU1ZpZXcgc2V0RnJh bWVTaXplOl0gKCkKIzI0IDB4MDAwMDAwMDEwMGYwYmJhNyBpbiAtW1dlYkZyYW1lVmlldyBzZXRG cmFtZVNpemU6XSAoc2VsZj0weDEwNjMyNDMyMCwgX2NtZD0weDdmZmY4NGMxNmVmOCwgc2l6ZT17 d2lkdGggPSAxMTgyLCBoZWlnaHQgPSAxMTI4fSkgYXQgV2ViS2l0L1dlYktpdC9tYWMvV2ViVmll dy9XZWJGcmFtZVZpZXcubW06NTA5CiMyNSAweDAwMDA3ZmZmODQ0ZmMxOWEgaW4gLVtOU1ZpZXcg c2V0RnJhbWU6XSAoKQojMjYgMHgwMDAwN2ZmZjg0NTdjMWE0IGluIC1bTlNWaWV3IHJlc2l6ZVdp dGhPbGRTdXBlcnZpZXdTaXplOl0gKCkKIzI3IDB4MDAwMDdmZmY4NDUwMjU1MCBpbiAtW05TVmll dyByZXNpemVTdWJ2aWV3c1dpdGhPbGRTaXplOl0gKCkKIzI4IDB4MDAwMDdmZmY4NDRmYzgzZSBp biAtW05TVmlldyBzZXRGcmFtZVNpemU6XSAoKQojMjkgMHgwMDAwMDAwMTAwZmIwYWEwIGluIC1b V2ViVmlldyhXZWJQcml2YXRlKSBzZXRGcmFtZVNpemU6XSAoc2VsZj0weDEwNjM5M2JkMCwgX2Nt ZD0weDdmZmY4NGMxNmVmOCwgc2l6ZT17d2lkdGggPSAxMTgyLCBoZWlnaHQgPSAxMTI4fSkgYXQg V2ViS2l0L1dlYktpdC9tYWMvV2ViVmlldy9XZWJWaWV3Lm1tOjgzNgojMzAgMHgwMDAwN2ZmZjg0 NGZjMTlhIGluIC1bTlNWaWV3IHNldEZyYW1lOl0gKCkKIzMxIDB4MDAwMDdmZmY4NDU3YzFhNCBp biAtW05TVmlldyByZXNpemVXaXRoT2xkU3VwZXJ2aWV3U2l6ZTpdICgpCiMzMiAweDAwMDA3ZmZm ODQ1MDI1NTAgaW4gLVtOU1ZpZXcgcmVzaXplU3Vidmlld3NXaXRoT2xkU2l6ZTpdICgpCiMzMyAw eDAwMDA3ZmZmODQ0ZmM4M2UgaW4gLVtOU1ZpZXcgc2V0RnJhbWVTaXplOl0gKCkKIzM0IDB4MDAw MDdmZmY4NDRmYzE5YSBpbiAtW05TVmlldyBzZXRGcmFtZTpdICgpCiMzNSAweDAwMDA3ZmZmODQ1 N2MxYTQgaW4gLVtOU1ZpZXcgcmVzaXplV2l0aE9sZFN1cGVydmlld1NpemU6XSAoKQojMzYgMHgw MDAwN2ZmZjg0NTAyNTUwIGluIC1bTlNWaWV3IHJlc2l6ZVN1YnZpZXdzV2l0aE9sZFNpemU6XSAo KQojMzcgMHgwMDAwN2ZmZjg0NGZjODNlIGluIC1bTlNWaWV3IHNldEZyYW1lU2l6ZTpdICgpCiMz OCAweDAwMDA3ZmZmODQ2ZWZlZGMgaW4gLVtOU1RhYlZpZXcgc2V0RnJhbWVTaXplOl0gKCkKIzM5 IDB4MDAwMDdmZmY4NDRmYzE5YSBpbiAtW05TVmlldyBzZXRGcmFtZTpdICgpCiM0MCAweDAwMDA3 ZmZmODQ1N2MxYTQgaW4gLVtOU1ZpZXcgcmVzaXplV2l0aE9sZFN1cGVydmlld1NpemU6XSAoKQoj NDEgMHgwMDAwN2ZmZjg0NTAyNTUwIGluIC1bTlNWaWV3IHJlc2l6ZVN1YnZpZXdzV2l0aE9sZFNp emU6XSAoKQojNDIgMHgwMDAwN2ZmZjg0NGZjODNlIGluIC1bTlNWaWV3IHNldEZyYW1lU2l6ZTpd ICgpCiM0MyAweDAwMDA3ZmZmODQ1MDA3YzQgaW4gLVtOU1dpbmRvdyBfb2xkUGxhY2VXaW5kb3c6 XSAoKQojNDQgMHgwMDAwN2ZmZjg0NGZmOWIzIGluIC1bTlNXaW5kb3cgX3NldEZyYW1lQ29tbW9u OmRpc3BsYXk6c3Rhc2hTaXplOl0gKCkKIzQ1IDB4MDAwMDAwMDEwMDAyNTA4ZiBpbiA/PyAoKQoj NDYgMHgwMDAwN2ZmZjg0YWE0ZmFkIGluIC1bTlNUaXRsZWRGcmFtZSByZXNpemVXaXRoRXZlbnQ6 XSAoKQojNDcgMHgwMDAwN2ZmZjg0NzE5NDE1IGluIC1bTlNUaXRsZWRGcmFtZSBtb3VzZURvd246 XSAoKQojNDggMHgwMDAwN2ZmZjg0NzE5MzdiIGluIC1bTlNUaGVtZUZyYW1lIG1vdXNlRG93bjpd ICgpCiM0OSAweDAwMDA3ZmZmODQ2MTc3NjMgaW4gLVtOU1dpbmRvdyBzZW5kRXZlbnQ6XSAoKQoj NTAgMHgwMDAwMDAwMTAwMDQxYmY1IGluID8/ICgpCiM1MSAweDAwMDAwMDAxMDAwNDFiODEgaW4g Pz8gKCkKIzUyIDB4MDAwMDdmZmY4NDU0Y2VlMiBpbiAtW05TQXBwbGljYXRpb24gc2VuZEV2ZW50 Ol0gKCkKIzUzIDB4MDAwMDAwMDEwMDAzODhjMiBpbiA/PyAoKQojNTQgMHgwMDAwN2ZmZjg0NGUz OTIyIGluIC1bTlNBcHBsaWNhdGlvbiBydW5dICgpCiM1NSAweDAwMDA3ZmZmODQ0ZGM1ZjggaW4g TlNBcHBsaWNhdGlvbk1haW4gKCkKIzU2IDB4MDAwMDAwMDEwMDAwOWIxOCBpbiA/PyAoKQ== 106932 2011-09-09 15:57:11 -0700 2011-09-09 15:57:11 -0700 Test case demonstrating what I was talking about in comment #11 beforeload.html text/html 411 marc.hoyois PCFET0NUWVBFIGh0bWw+CjxoZWFkPgo8L2hlYWQ+Cjxib2R5PgoKPHNjcmlwdD4KdmFyIGkgPSAw Owpkb2N1bWVudC5hZGRFdmVudExpc3RlbmVyKCJiZWZvcmVsb2FkIiwgZnVuY3Rpb24oZXZlbnQp IHsKCWlmKCEoZXZlbnQudGFyZ2V0IGluc3RhbmNlb2YgSFRNTEVtYmVkRWxlbWVudCkpIHJldHVy bjsKCXZhciBqID0gKytpOwoJY29uc29sZS5sb2coaiArICIgYmVmb3JlIGFjY2Vzc2luZyBwYXJl bnROb2RlIik7CglldmVudC50YXJnZXQucGFyZW50Tm9kZS5zb21lcHJvcGVydHk7Cgljb25zb2xl LmxvZyhqICsgIiBhZnRlciBhY2Nlc3NpbmcgcGFyZW50Tm9kZSIpOwp9LCB0cnVlKTsKPC9zY3Jp cHQ+Cgo8b2JqZWN0PgoJPGVtYmVkIHR5cGU9InZpZGVvL2Fxa3Fzc2QiPgo8L29iamVjdD4KCjwv Ym9keT48L2h0bWw+