Last Comment Bug 335251 - Alert in a loop
: Alert in a loop
Status: RESOLVED FIXED
This bug hangs http://www.idg.se and ...
: hang, regression
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- critical with 1 vote (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: Hixie (not reading bugmail)
: Andrew Overholt [:overholt]
Mentors:
http://thedailywtf.com/forums/AddPost...
: 338839 (view as bug list)
Depends on:
Blocks: 234455 331374 333163 335265 339337 339516 343367 344261
  Show dependency treegraph
 
Reported: 2006-04-24 07:43 PDT by Ria Klaassen (not reading all bugmail)
Modified: 2008-01-24 07:40 PST (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
possible patch (2.17 KB, patch)
2006-04-29 13:55 PDT, Olli Pettay [:smaug]
bzbarsky: review+
jst: superreview+
Details | Diff | Splinter Review

Description Ria Klaassen (not reading all bugmail) 2006-04-24 07:43:51 PDT
Steps to reproduce:
- Go to http://thedailywtf.com/forums/AddPost.aspx?PostID=69457
- Click on the Cut button
- An alert pops up and the problem is, that it doesn't let itself click away. 
You need to call the taskmanager.

Regression between 1.9a1_2006030704 and 1.9a1_2006030713.
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-03-07+03%3A00&maxdate=2006-03-07+13%3A00

Could be caused by Bug 234455.
Comment 1 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-04-24 09:36:26 PDT
This bug was discovered in bug 335216.
Comment 2 Olli Pettay [:smaug] 2006-04-24 13:56:37 PDT
I think the problem is FTB_AddEvent in http://thedailywtf.com/FreeTextBox3/FTB-Utility.js. 
It adds event listeners to capture phase and in the html page
there is FTB_AddEvent(window,'load',function () { ...
That means that for each image on the page that function is called.
(You can get rid of the alert if you click on it *many* times.)

See also Bug 331306.

Comment 3 Olli Pettay [:smaug] 2006-04-24 14:55:59 PDT
Btw, even if I added a hack to prevent the capture phase of image load events, there would be a new bug when Bug 235441 gets fixed.
http://thedailywtf.com/FreeTextBox3/FTB-Utility.js really should use
bubble phase I think, not capture phase.

(need to still test whether it works when adding listeners to bubble phase)
Comment 4 Olli Pettay [:smaug] 2006-04-29 13:55:15 PDT
Created attachment 220253 [details] [diff] [review]
possible patch

I think it makes sense to prevent load events to propagate to |window|, |document| is enough.
With this change load events still work according to DOM (at least IMO, since
events go from document to target).
In 1.8 load events have only AT_TARGET phase, and that is Bug 331306.

This fixes the Alert loop problem in this bug, and also Bug 333163 and Bug 335265.
Comment 5 Olli Pettay [:smaug] 2006-04-29 14:01:51 PDT
Comment on attachment 220253 [details] [diff] [review]
possible patch

In a way this is one suggestion for Bug 329514.
Comment 6 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-04-29 20:32:29 PDT
I'm not super happy about doing this. It's always bad with inconsistencies. Additionally, the window spec that is in the works will define that events propagate through the window object.

Of course, if we have to do it in order to not break a lot of sites then we'll just suck it up and do it anyway. But I don't think we should do it unless we have to.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2006-04-30 21:12:44 PDT
Comment on attachment 220253 [details] [diff] [review]
possible patch

Hmm.... I guess I'm ok with this, but I'd like jst to take a look too.
Comment 8 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-05-01 01:46:33 PDT
So there are a lot of sites that are affected by this? We can't just evangalize this one site?
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2006-05-01 08:41:26 PDT
There are at least 3 -- see the dep bugs.  Not sure whether we still do want to try evangelizing them.
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2006-05-02 21:54:18 PDT
How exactly did this regress (I'm assuming it regressed from smaug's big event dispatch cleanup changes)? What changed?
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2006-05-02 22:03:55 PDT
We started capturing image load events.  Before the event change they did not propagate at all -- no bubble, no capture.  Now they just don't bubble; they do capture.
Comment 12 Olli Pettay [:smaug] 2006-05-02 22:06:43 PDT
in 1.8 load events have only at_target phase. After event dispatching rewrite
those events have the capture phase too (which is according to DOM).
Comment 13 mjbjr 2006-05-03 10:50:17 PDT
The 'Alert' popup window appears to be modal, preventing the killing of the
tab containing the offending site, hence then loop.  At the minimum, can't
the 'Alert' popup window made non-modal.

A vast majority of the time that I run across a modal widget, it doesn't _need_
to be.  IMO, modal is used way too much.  Modal as default is a Windows thing
that's been carried over.

I ran across these problems when trying to view an article
"California governor promotes Open Office" on digg.(In reply to comment #0)
> Steps to reproduce:
> - Go to http://thedailywtf.com/forums/AddPost.aspx?PostID=69457
> - Click on the Cut button
> - An alert pops up and the problem is, that it doesn't let itself click away. 
> You need to call the taskmanager.
> 
> Regression between 1.9a1_2006030704 and 1.9a1_2006030713.
> http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-03-07+03%3A00&maxdate=2006-03-07+13%3A00
> 
> Could be caused by Bug 234455.
> 

Comment 14 Alex Vincent [:WeirdAl] 2006-05-04 01:49:38 PDT
:( I think this patch sucks, badly.  1.9, which this is targeted at, is still over a year away.  If the webpage is broken, it's broken.

What do other browsers do?  IE doesn't support DOM events to my knowledge (IE7?)
Comment 15 Alex Vincent [:WeirdAl] 2006-05-04 01:59:00 PDT
Also, how hard is it for a webpage to check where the event is targeted?
Comment 16 Olli Pettay [:smaug] 2006-05-04 02:16:07 PDT
It is not hard to check the target, but that is not done, which is 
the reason for these regressions.
All those pages have separate code for IE event handling.
Comment 17 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-05-04 03:25:29 PDT
Could we at least try to evangelize this before checking it in? If we just give up we're not going to be able to fix the bug about firing capturing EventListeners at the target (can't find the bug right now)
Comment 18 Olli Pettay [:smaug] 2006-05-05 01:14:47 PDT
(In reply to comment #17)
> Could we at least try to evangelize this before checking it in? If we just give
> up we're not going to be able to fix the bug about firing capturing
> EventListeners at the target (can't find the bug right now)
> 

true. :( Capturing event listeners bug is Bug 235441.
Is there any place where we could collect all the (1.9) changes which affect somehow web developers? 
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2006-05-05 08:07:08 PDT
A page on developer.mozilla.org is the place to go, imo.  It'll be a _long_ list.  :(
Comment 20 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-05-15 04:44:21 PDT
I've started a page here:
http://developer.mozilla.org/en/docs/Gecko_1.9_Changes_for_web_developers
I don't know too much about the subject, so please correct me where I'm wrong (or if I forgot to mention something).
Comment 21 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-05-15 04:56:30 PDT
Sorry, I just moved the page to:
http://developer.mozilla.org/en/docs/Gecko_1.9_Changes_affecting_websites
(I think that is a better name)
Comment 22 Olli Pettay [:smaug] 2006-05-17 05:18:36 PDT
*** Bug 335265 has been marked as a duplicate of this bug. ***
Comment 23 Ria Klaassen (not reading all bugmail) 2006-05-22 11:08:42 PDT
*** Bug 338839 has been marked as a duplicate of this bug. ***
Comment 24 Håkan Waara 2006-05-27 06:57:14 PDT
Just fyi, this breaks sweden's biggest computer news site (hang & 100% CPU), so pretty critical for us. :-)

Patch still waiting for super-review (from jst).
Comment 25 Robert Claypool 2006-05-29 14:16:18 PDT
It seems that the alert is produced because a message is being sent where there isn't a listener. So don't send messages to levels where there isn't a listener attached?
Comment 26 Boris Zbarsky [:bz] (still a bit busy) 2006-05-29 14:23:35 PDT
The first sentence of comment 25 is incorrect.
Comment 27 Håkan Waara 2006-07-06 05:17:44 PDT
Is the patch attached here just waiting for super-review?
Comment 28 Johnny Stenback (:jst, jst@mozilla.com) 2006-07-11 17:54:04 PDT
Comment on attachment 220253 [details] [diff] [review]
possible patch

I'm not excited about this, but I also don't see a better way out. sr=jst
Comment 29 Olli Pettay [:smaug] 2006-07-12 12:24:16 PDT
Checked in.

I'm really not at all happy with the fix, but apparently there are just too
many websites relying on the behaviour that only page load event is propagated to
|window| :(
Comment 30 Ger Teunis 2006-08-02 06:38:45 PDT
On the latest build the above page does get in an infinite (reload) loop. 
I think it is related to this fix.

1. Open the URL http://thedailywtf.com/forums/AddPost.aspx?PostID=69457
2. Click on the top-left logo, wait for the load to complete
3. Use any back button

The page will infinite reload, if this is not happening, try again.
Around 50%+ of the times I've tries I seed the reload happening.
Comment 31 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-08-02 07:20:42 PDT
(In reply to comment #30)
Ger, could you please file a new bug on that? And mention the bug number here?
Comment 32 Ger Teunis 2006-08-02 07:31:03 PDT
Filed: "Message editor on thedailywtf.com forums causes an infinite reload loop when using back button"
Bug#347003.
Comment 33 Hallvord R. M. Steen 2006-12-24 05:51:02 PST
My comment:
http://my.opera.com/hallvors/blog/2006/12/23/firechicken
Comment 34 Joao Eiras 2007-01-22 23:41:51 PST
> I'm really not at all happy with the fix, but apparently there
> are just too many websites relying on the behaviour that only
> page load event is propagated to |window|

Indeed there are many websites that break. Still a big inconsistency has been introduced in the event model, considering that the window is part of the event flow. I seriously suggest for you to work with the W3C team which is developing the document around the Window object, and either revert you behaviour to keep the event model logical, or force this change in the Window specification. There's a problem though. I don't know if Safari supports capturing load event listeners but Opera has have a hard time for websites using uneeded capture. You're basically pointing your middle finger at them :p
So far FF3 is far away from a release, so you can either introduce the correct behaviour right away and try to raise awareness of the important changes in Gecko, and have webmaster testing your websites in public betas (they don't like nightlies), or wait until a bit before the release and introduce the change so developers will see the problems with public releases and hurry to fix their problems, which will hurt users, only in a short run.

Websites using unwanted capture are a matter for Tech Envagelism, not bug fixing. The same happened for throwing WRONG_DOCUMENT_ERR which the Yahoo people accepted, and fixed the problem on their side.
Now I'm waiting for bug 235441 to get fixed. This is clearly against the DOM spec, and webmasters have no choice but to sniff browsers because the specification as different behaviour in different browsers, hurting interoperability. The Safari team turned their backs on the DOM spec and implemented bug 235441 for the sake of compatibility with existing content. I really don't see that as a good idea.
Comment 35 Boris Zbarsky [:bz] (still a bit busy) 2007-01-23 00:05:21 PST
Joao, last I checked we _were_ following this discussion in the W3C.

> So far FF3 is far away from a release

9 months ain't that far, actually...   If all goes to plan, of course.

> The same happened for throwing WRONG_DOCUMENT_ERR

That was just a bug, pure and simple.  But note that due to IE's behavior with XMLHttpRequest we will need to do _something_ about it.  There's no script out there that does the right thing with XMLHttpRequest right now.

> Now I'm waiting for bug 235441 to get fixed.

Which has nothing to do with this bug.

In fact, please see https://bugzilla.mozilla.org/etiquette.html in general.  ;)

Comment 36 Joao Eiras 2007-01-23 00:08:11 PST
> Joao, last I checked we _were_ following this discussion in the W3C.
Yes we were, but not everyone CCed of this bug reads webapi's mail... I think.

> 9 months ain't that far
I thought it was more. 9 months ? Great.


> In fact, please see https://bugzilla.mozilla.org/etiquette.html in general.  ;)

My apologies, I'm well aware of that :)

Keep up.

Note You need to log in before you can comment on or make changes to this bug.