Bug 7657 – REGRESSION: digg.com comments page so slow it seems like a hang (image load events mistaken for page load)

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

[Adam Elliot]

http://digg.com/linux_unix/IBM_Not_To_Use_Vista_-_But_Will_Move_to_Linux_Desktops

The page loads mostly and then Safari hangs.  This doesn't seem to be happening on the current release version of Safari just the last few nightly builds I've checked.

[Adam Elliot]

Created attachment 6934 [details]
This web archive seems to reproduce the crash

As the page is a comment page it may change and the crash might go away this archive will reproduce the bug.  If someone else could verify that it's not just me having the issue that'd be great.

[Alexey Proskuryakov]

Appears to freeze in layout code (ElementImpl::recalcStyle())

This is not a freeze. If you wait long enough, the page will finish loading. It executes the window's load event listener for every image loaded on the page, and that listener adds style rules, which invokes layout code. I doubt that the listener is intended to run so many times.

The page sets up a capturing listener for load events on the window. In Firefox, such a listener is called only for the document's load event (behavior in Firefox is the same if the listener is non-capturing), whereas in WebKit it is called for the images' load events.

Created attachment 6946 [details]
Test case

This shows that in WebKit, the listener is called with the image's load event. In Firefox, it is called with the document's.

[Adam Elliot]

If this is the intended behavior then it seems that there is a new problem where pages the are implemented using this feature will now have incredibly lengthly load times.  Any ideas on what can be done to prevent existing pages not appear to crash?

[Maciej Stachowiak]

image load events should not bubble, if that is happening in WebKit it is a serious bug.

(In reply to comment #7)
> image load events should not bubble, if that is happening in WebKit it is a
> serious bug.
> 

That's not the case here. The listener is set up as useCapture. I don't see that WebKit is doing anything wrong, yet it differs from Firefox.

[Darin Adler]

When Maciej says the events should not bubble, maybe he means they also should not propagate during the capture phase either.

[Darin Adler]

I tried to read the HTML 4 and DOM Level 2 specifications to clear this up. Both talk about the load event, but neither mentions image elements in this context. The HTML 4 specification <http://www.w3.org/TR/html401/interact/scripts.html> mentions only body elements and frameset elements. The DOM Level 2 specification <http://www.w3.org/TR/DOM-Level-2-Events/events.html> adds a mention of object elements. Neither mentions load events for image elements, so they are not helpful to resolve this issue.

[Alice Liu]

<rdar://problem/4483864>

[Maciej Stachowiak]

The DOM Level 2 Events specification pretty clearly says that all events in that spec (which include "load") should do capture. Also, Mozilla has now fixed the bug that caused image load events not to capture: https://bugzilla.mozilla.org/show_bug.cgi?id=331306

So I think we probably should not emulate this old Mozilla bug, even though digg.com seems to rely on it.

[Maciej Stachowiak]

The simple workaround for digg.com would be to pass false for the useCapture flag. This will work with older Mozilla versions as well as with Safari and future standards-compliant versions of Mozilla.

[Steven Fisher]

Even if it is the Digg site doing something stupid, Safari spinning the busy cursor for over 30 seconds is a problem.

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

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

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

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

[Kyle Hayes]

Digg.com has recently changed the way their comment system works so it could be partially related to that also.

[Eric Seidel (no email)]

I spoke with one of the gentleman from Digg while at SuperHappyDevHouse. Hopefully this will be resolved shortly.

[Alexey Proskuryakov]

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

[Steven Fisher]

Reading comment #20 above, I'm a little concerned here that this is being viewed as a digg-only problem. That a site can cause WebKit to lock up for over a minute with no way for the user to cancel the operation is going to be a problem, even if it is caused by a bad website.

[Darin Adler]

(In reply to comment #22)
> Reading comment #20 above, I'm a little concerned here that this is being
> viewed as a digg-only problem. That a site can cause WebKit to lock up for over
> a minute with no way for the user to cancel the operation is going to be a
> problem, even if it is caused by a bad website.

Bug 7080 addresses the general issue of JavaScript causing WebKit to lock up.

[Steven Fisher]

Thank you for the clarification -- that makes a lot more sense to me.

[Trevor Harmon]

(In reply to comment #23)
> Bug 7080 addresses the general issue of JavaScript causing WebKit to lock up.

Does that bug actually apply to this one? Note that WebKit doesn't lock up on Digg; it just hangs for awhile (a couple minutes) then returns to normal. I don't think an infinite JavaScript loop is causing the problem.

[Darin Adler]

(In reply to comment #25)
> Does that bug actually apply to this one? Note that WebKit doesn't lock up on
> Digg; it just hangs for awhile (a couple minutes) then returns to normal. I
> don't think an infinite JavaScript loop is causing the problem.

Maybe.

The bug here is that due to a bug in the site, there's JavaScript code runs over and over again. That's what's so slow.

From the point of view of the "stop JavaScript infinite loop" code, there's no difference between code that takes a long time to run and code that loops forever.

On the other hand, it's not clear if our solution for that will work well for a case where the slowness is due to many separate slow JavaScript event handlers as opposed to one single event. I think the best answer is to revisit this once we have that problem solved. Although there may be a separate issue, I don't think that yet another separate bug report would help matters.

[Richard Das]

(In reply to comment #26)
> From the point of view of the "stop JavaScript infinite loop" code, there's no
> difference between code that takes a long time to run and code that loops
> forever.

I don't think it's up to webkit dev to "guard" against what could be described as faulty programming (it will always be possible to write buggy code!). In this case, it's a problem with how digg was coded. They seem to have fixed the problem now however, as I am no longer experiencing the long-wait.

(In reply to comment #12)
> So I think we probably should not emulate this old Mozilla bug, even though
> digg.com seems to rely on it.

It seems like while this thread is directly related to digg, the "bug" in question is a more generic interpretation of how load events are handled. So long as this isn't a bug in how WebKit is actually running the code it's given, can we mark this bug as closed?

[Steven Fisher]

I really have to disagree. Just as it is the responsibility of the operating system to guard against faulty programming in an application taking down the entire operating system, it is without question the responsibility of the browser to guard against faulty programming within a web application taking down the browser.

I just checked digg. It's still quite slow, but it is no longer so slow that it seems to have crashed the browser.

[Richard Das]

fair enough.

I meant that we should perhaps close this bug, as it's specifically related to digg, and continue discussion of this issue in a more broad thread regarding how webkit should handle load events.

In any case, we seem to have narrowed down the source of the bug to load events, and at least it's not just some "random behavior"! :) Now how do we go about establishing a safeguard?

[Steven Fisher]

I'm not sure I consider the current behavior on digg "fixed" as 15-20 seconds of spin still seems fairly bad to me, but it is probably fixed enough for most people. And I agree that it makes more sense to have continued discussion in a new (or at least different) defect, with a and more focused test case so on. :)

[Darin Adler]

If there are issues separate from the load event one (which no longer affects digg), then lets get separate bug reports.

For example, if the site is too slow, then we can consider other improvements to WebKit to make it faster. We'll need a separate bug report for that.

[Darin Adler]

This particular regression is no longer an issue on this particular web page. New bugs about pages with a regression in performance (slower on nightly builds than on stock Safari), or about additional slowness on this page that is not a regression, would be welcome.