Closed Bug 335251 Opened 17 years ago Closed 17 years ago

Alert in a loop

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: ria.klaassen, Assigned: smaug)

References

(
URL
)

Details

(Keywords: hang, regression, Whiteboard: This bug hangs http://www.idg.se and http://www.mapquest.com)

Attachments

(1 file)

possible patch
2.17 KB, patch
bzbarsky
: review+
jst
: superreview+
Details | Diff | Splinter Review 
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.
This bug was discovered in bug 335216.
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.


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)
Blocks: 333163
Blocks: 335265
Attached patch possible patch — — Splinter Review 
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.
Assignee: events → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #220253 - Flags: superreview?(bzbarsky)
Attachment #220253 - Flags: review?(bzbarsky)
Comment on attachment 220253 [details] [diff] [review]
possible patch

In a way this is one suggestion for Bug 329514.
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 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.
Attachment #220253 - Flags: superreview?(jst)
Attachment #220253 - Flags: superreview?(bzbarsky)
Attachment #220253 - Flags: review?(bzbarsky)
Attachment #220253 - Flags: review+
So there are a lot of sites that are affected by this? We can't just evangalize this one site?
There are at least 3 -- see the dep bugs.  Not sure whether we still do want to try evangelizing them.
How exactly did this regress (I'm assuming it regressed from smaug's big event dispatch cleanup changes)? What changed?
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.
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).
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.
> 


:( 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?)
Also, how hard is it for a webpage to check where the event is targeted?
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.
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)
Blocks: 331374
(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? 

A page on developer.mozilla.org is the place to go, imo.  It'll be a _long_ list.  :(
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).
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)
*** Bug 335265 has been marked as a duplicate of this bug. ***
*** Bug 338839 has been marked as a duplicate of this bug. ***
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).
Severity: normal → critical
Keywords: hang
OS: Windows XP → All
Hardware: PC → All
Whiteboard: This bug hangs http://www.idg.se and http://www.mapquest.com
Blocks: 339337
Blocks: 339516
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?
The first sentence of comment 25 is incorrect.
Blocks: 343111
Blocks: 343367
Is the patch attached here just waiting for super-review?
Blocks: 344261
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
Attachment #220253 - Flags: superreview?(jst) → superreview+
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| :(
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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.
(In reply to comment #30)
Ger, could you please file a new bug on that? And mention the bug number here?

Filed: "Message editor on thedailywtf.com forums causes an infinite reload loop when using back button"
Bug#347003.
> 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.
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.  ;)


> 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.
You need to log in before you can comment on or make changes to this bug.