Bug 45586 - Having an empty listener to beforeload events changes the behavior of other scripts
: Having an empty listener to beforeload events changes the behavior of other s...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.6
: P2 Major
Assigned To: Andy Estes
http://odyniec.net/projects/imgzoom/
: InRadar
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-10 22:57 PDT by Marc Hoyois
Modified: 2014-12-02 09:28 PST (History)
13 users (show)

See Also:


Attachments
A trivial Safari extension reproducing the bug (4.97 KB, application/x-safari-extension)
2010-09-10 22:57 PDT, Marc Hoyois
no flags Details
Minimal testcase (3.54 KB, text/html)
2010-12-17 15:06 PST, Wladimir Palant
no flags Details
Patch (5.77 KB, patch)
2011-09-13 01:52 PDT, Andy Estes
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Hoyois 2010-09-10 22:57:29 PDT
Created attachment 67292 [details]
A trivial Safari extension reproducing the bug

Overview:

Simply registering a beforeload event listener in an injected script breaks the imgZoom jQuery plugin.

Steps to reproduce:
1. Install the attached extension (and disable any other extension if you wish). It has the following trivial code in an injected script:

document.addEventListener("beforeload", function(){}, true);

2. Navigate to http://odyniec.net/projects/imgzoom/

3. Click on one of the three images. The image should enlarge.

4. Reload the page manually.

5. Click on the same image again.

Actual result:
The image shrinks.

Expected result:
The image should be enlarged as in step 3.

Platform:
Tested on OS X 10.6.4 with both Safari 5.0.2 and Webkit 6533.18.5, r67077

Note:
Theoretically, this extension should have no effect whatsoever. The same bug happens with nontrivial event handlers.
Comment 1 Alexey Proskuryakov 2010-09-13 14:18:38 PDT
<rdar://problem/8424354>
Comment 2 Marc Hoyois 2010-09-22 08:07:04 PDT
The issue has been resolved by imgZoom 0.2.1. What was going on is that the script was using an image's offsetWidth property in a handler to the image's 'load' event. Somehow the presence of any beforeload event listener delays the setting of offsetWidth to just after the handler is called (as can be seen using a setTimeout of 0 milliseconds), but it is not set within the handler (i.e. equals 0). Since the spec does not require .offsetWidth to be set when the 'load' event is fired (the image's .width property has this purpose and works correctly), this behavior seems legit, although the fact that it happens exclusively with beforeload listeners indicates that it might be the consequence of another bug.
Comment 3 Aaron Boodman 2010-10-05 18:50:35 PDT
(In reply to comment #2)
> Since the spec does not require .offsetWidth to be set when the 'load' event is fired (the image's .width property has this purpose and works correctly), this behavior seems legit, although the fact that it happens exclusively with beforeload listeners indicates that it might be the consequence of another bug.

I disagree that the behavior is legit. It appears that the behavior without anyone listening to the 'beforeLoad' event is that the width/height are always set in the 'load' event. I believe that is also the behavior in other browsers.

Therefore the result of this bug is that extension developers can break web compat in a very unexpected way that would be difficult for them to anticipate, or to do anything about.
Comment 4 Wladimir Palant 2010-12-17 14:55:38 PST
The actual issue is that the "load" event for the image fires too early, before the image loads. In fact, the "load" event fires even before the "beforeload" event, you can clearly see it by putting console.log() calls into both event handlers. So you will always get a successful "load", even for an image that doesn't exists. And this is clearly not by the spec, I don't think there are any doubts about that.
Comment 5 Alexey Proskuryakov 2010-12-17 15:06:37 PST
> the "load" event for the image fires too early, before the image loads. In fact, the "load" event fires even before the "beforeload" event

Can you reproduce this with Web content (without an extension)? Could you please attach a test case?
Comment 6 Wladimir Palant 2010-12-17 15:06:55 PST
Created attachment 76914 [details]
Minimal testcase

Minimal testcase attached - you don't need an extension, a regular webpage will do. The first time you click the "load image" button (image not in cache yet) you get the correct output to the console:

document's beforeload event, image width = 0
image's load event, image width = 100

Note that we *first* have "beforeload" and only then the "load" event. Second time you get:

image's load event, image width = 0
document's beforeload event, image width = 0

Here and in all the subsequent runs you get the wrong behavior, "load" event fires before "beforeload" event. To get the original behavior you have to clear the browser cache, this bug apparently only happens for cached images.

Tested with Chrome 8.0.552.224.
Comment 7 Alexey Proskuryakov 2010-12-17 15:18:18 PST
Nasty.
Comment 8 Paul Harvey 2011-08-21 04:17:44 PDT
This is still a significant bug that needs attention.

See:
http://code.google.com/p/google-ajax-apis/issues/detail?id=566
http://code.google.com/p/adblockforchrome/issues/detail?id=5821
Comment 9 Paul Buonopane 2011-08-25 14:15:36 PDT
I am confirming that this is still a major bug, and much more so than the original report would have one believe.

I am developing a rather large, multi-browser extension for an advertising company that blocks ads.  (How that makes sense is a long story, and quite ingenious, but unnecessary for this discussion.)  To summarize my findings, adding a trivial beforeload handler, at least as part of an extension, disables numerous scripts across the Web.

Examples of notable sites that have become unusable after a trivial beforeload listener is installed include the Amazon AWS management console and Google Mail.

This bug is currently known to inhibit the abilities of AdBlock Plus, a popular extension.  I discovered this while looking to see how AdBlock Plus works around the bug.  It doesn't.  It can't.  Rather than blocking resources from loading as it should and could if it weren't for this bug, it has to manually go through each element and remove any that attempts to load a blocked resource: a much less efficient method.

As long as this bug persists, there is no way for extensions such as AdBlock Plus and my own to prevent resources from loading without predictably breaking an unacceptable number of web pages.
Comment 10 Wladimir Palant 2011-08-31 07:59:54 PDT
Just realized that having the "beforeload" event handler around isn't necessary to get broken behavior. It is enough that the event handler was registered at some point - removing it again won't help you any more, the "load" event for images will still fire too early. Which severely limits the work-arounds one can try.
Comment 11 Paul Buonopane 2011-08-31 09:14:05 PDT
(In reply to comment #10)
> Just realized that having the "beforeload" event handler around isn't necessary to get broken behavior. It is enough that the event handler was registered at some point - removing it again won't help you any more, the "load" event for images will still fire too early. Which severely limits the work-arounds one can try.

Wladimir Palant, I noticed that AdBlock Plus simply disables the majority of its checks for a hardcoded list of sites known to break because of this bug.  Would it be instead possible to re-fire the load events for each element after a certain delay?  I have yet to try this approach, but it seems plausible.

I did some more research into how this breaks Google Mail and the AWS Management Console.  It looks as though I was causing this bug to be more severe by triggering another bug.  I can't give many more details than that, as I have yet to investigate this other bug, but the point is that the bug on which we are focusing can become even more severe when coupled with other bugs or even certain JavaScript techniques.  For example, after adding a listener for beforeload, any modifications to an element's innerHTML property can have odd effects.
Comment 12 Wladimir Palant 2011-08-31 09:17:25 PDT
(In reply to comment #11)
> Would it be instead possible to re-fire the load events for each element after a certain delay?

I tried that a while ago - it had no effect, the scripts in question only listen for the first "load" event (and it cannot be prevented).
Comment 13 Alexey Proskuryakov 2011-08-31 09:48:43 PDT
Also CC'ing Sam, who recently did some work on image load events.
Comment 14 Andy Estes 2011-09-08 13:50:41 PDT
I'm going to take a look at this today and see if I can make some progress. This bug has been assigned for almost a year but there doesn't seem to be any patch activity, so I assume I won't be stepping on anyone's feet.
Comment 15 Andy Estes 2011-09-08 13:53:00 PDT
(In reply to comment #12)
> (In reply to comment #11)
> > Would it be instead possible to re-fire the load events for each element after a certain delay?
> 
> I tried that a while ago - it had no effect, the scripts in question only listen for the first "load" event (and it cannot be prevented).

I believe this is due to <https://bugs.webkit.org/show_bug.cgi?id=33861>. We add events to the document's listener map on calls to addEventListener() but do not remove them on calls to removeEventListener().
Comment 16 Andy Estes 2011-09-08 13:55:32 PDT
(In reply to comment #15)
> I believe this is due to <https://bugs.webkit.org/show_bug.cgi?id=33861>. We add events to the document's listener map on calls to addEventListener() but do not remove them on calls to removeEventListener().

Sorry, this was meant to be in response to Comment #10.
Comment 17 Andy Estes 2011-09-08 14:12:07 PDT
I think this is due to this block of code starting at <http://trac.webkit.org/browser/trunk/Source/WebCore/loader/ImageLoader.cpp#L204>:

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

If newImage is cached, then the call to addClient() will cause the load event to fire *before* we dispatch the beforeload event. Not only is this the wrong order of events, but since dispatchPendingBeforeLoadEvent() is what ends up wiring up the cached image to the RenderImage, someone inspecting the image DOM object in an unload handler will see incorrect information.
Comment 18 Andy Estes 2011-09-08 14:13:10 PDT
s/unload/onload
Comment 19 Andy Estes 2011-09-08 14:17:14 PDT
In the case where Document::hasListenerType(Document::BEFORELOAD_LISTENER) returns false, things work correctly since the beforeload event is fired synchronously before the load event which is on a 0-delay timer. This bug is all about the case where hasListenerType() returns true and the beforeload event is also fired on a timer that happens to get queued behind the load event.
Comment 20 Andy Estes 2011-09-08 15:03:00 PDT
I think we're just calling setClient() too early. I'm testing a patch.
Comment 21 Andy Estes 2011-09-13 01:26:08 PDT
*** Bug 65388 has been marked as a duplicate of this bug. ***
Comment 22 Andy Estes 2011-09-13 01:52:43 PDT
Created attachment 107152 [details]
Patch
Comment 23 mitz@webkit.org 2011-09-13 07:27:57 PDT
Comment on attachment 107152 [details]
Patch

Sorry, I didn’t mean to r+ this. (Didn’t mean to r- either!).
Comment 24 Darin Adler 2011-09-13 08:35:35 PDT
Comment on attachment 107152 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107152&action=review

> Source/WebCore/loader/ImageLoader.cpp:209
>              if (!m_element->document()->hasListenerType(Document::BEFORELOAD_LISTENER))
>                  dispatchPendingBeforeLoadEvent();
>              else
>                  beforeLoadEventSender().dispatchEventSoon(this);
> +            newImage->addClient(this);

I think it’s worth adding a comment here. It’s probably not obvious to most readers that addClient often results in a load event dispatch and that the order here is important so that beforeload goes before load. A brief "why" comment could make that clear without making the code unpleasant.
Comment 25 Andy Estes 2011-09-15 14:20:10 PDT
Committed r95228: <http://trac.webkit.org/changeset/95228>
Comment 26 Paul Harvey 2011-09-15 17:42:59 PDT
Superb!  Thanks so much for your efforts, Andy.

When can we expect this to be rolled out to the average web user?

Thanks again,
PaulH
Comment 27 Andy Estes 2011-09-15 19:14:09 PDT
Hi Paul,

It's not really possible for me to answer your question. All I can say is that this fix is in WebKit trunk and can be tested in WebKit nightly builds. It's up to the downstream vendors of WebKit-based products, not the WebKit Open Source Project itself, to decide if and when a version of WebKit that includes this fix is shipped to customers.

-Andy
Comment 28 Peter Kasting 2011-11-05 18:24:51 PDT
(In reply to comment #26)
> When can we expect this to be rolled out to the average web user?

I can't speak for other vendors, but for Chrome, this should be present in Chrome 16+, which currently corresponds to the Chrome beta, dev, and canary channels.

If you ever want to check this sort of thing you can visit http://omahaproxy.appspot.com/viewer .  Note the "base webkit revision" and compare to the revision number in comment 25.
Comment 29 famlam 2011-12-13 15:13:23 PST
Hi Andy,
Thanks for fixing this!

It unfortunately looks like a small part of this bug isn't fixed yet. I filed that in bug 74451. I'm not 100% sure about the relation to this issue, but it has a lot of similarities.

Kind regards,
Famlam