Bug 45586 – Having an empty listener to beforeload events changes the behavior of other scripts

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Marc Hoyois]

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.

[Alexey Proskuryakov]

<rdar://problem/8424354>

[Marc Hoyois]

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.

[Aaron Boodman]

(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.

[Wladimir Palant]

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.

[Alexey Proskuryakov]

> 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?

[Wladimir Palant]

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.

[Alexey Proskuryakov]

Nasty.

[Paul Harvey]

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

[Paul Buonopane]

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.

[Wladimir Palant]

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.

[Paul Buonopane]

(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.

[Wladimir Palant]

(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).

[Alexey Proskuryakov]

Also CC'ing Sam, who recently did some work on image load events.

[Andy Estes]

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.

[Andy Estes]

(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().

[Andy Estes]

(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.

[Andy Estes]

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.

[Andy Estes]

s/unload/onload

[Andy Estes]

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.

[Andy Estes]

I think we're just calling setClient() too early. I'm testing a patch.

[Andy Estes]

*** Bug 65388 has been marked as a duplicate of this bug. ***

[Andy Estes]

Created attachment 107152 [details]
Patch

Comment on attachment 107152 [details]
Patch

Sorry, I didn’t mean to r+ this. (Didn’t mean to r- either!).

[Darin Adler]

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.

[Andy Estes]

Committed r95228: <http://trac.webkit.org/changeset/95228>

[Paul Harvey]

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

[Andy Estes]

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

[Peter Kasting]

(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.

[famlam]

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