196057 - xul window onload event fired twice if iframe is contained

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Karsten Düsterloh]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4a) Gecko/20030305
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4a) Gecko/20030305

When writing XUL pages with the root XUL elements window/page/dialog, the onload
event is fired twice if an iframe exists in it. If no iframe is given, the event
fires once. If the iframe itself has an onload handler, three events are
processed: one for the iframe, two for the root element.

This bug must actually be around at least a year now, it is the reason for bug
147068.

Reproducible: Always

Steps to Reproduce:
 



(Testcase follows.)

Comment 1

[Karsten Düsterloh]

Attachment: XUL window with iframe and onload alerts

same bug with page/dialog instead of window

Comment 2

[Boris Zbarsky [:bzbarsky]]

Sounds like something broke and the onload event is bubbling....

Comment 3

[Karsten Düsterloh]

Bug 147068 has a simple workaround to circumvent this bug here, else bug 195885
would have been stuck. The workaround should be removed as soon as this bug here
is resolved.

Comment 4

[Karsten Düsterloh]

As Boris stated in a posting to npm.dom:
> load events do not bubble, per DOM2 Events.
> See
http://www.w3.org/TR/2000/REC-DOM-Level-2-Events-20001113/events.html#Events-eventgroupings-htmlevents

Comment 5

[[e-mail]]

The onload events that are firing on the iframe and bubbling up to the window
don't have a target! The third event that fires on the window's document is
correctly targetted at that document.

Question: should you even be able to listen to a load of a frame using an
bubbling event handler on the frame's container element?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Per DOM spec, no.  Does it work with HTML?  I would not be one bit surprised if
this is a XUL hack...

Comment 7

[[e-mail]]

OK, so using this html the main document actually loaded first - once...
Then the iframe loads, and its body event handler target is its document.
Then the iframe element onload handler fires, but with a null target.
I can reload the iframe which fires the same two load events.

Comment 8

[Boris Zbarsky [:bzbarsky]]

hmm... That looks like in HTML the load event is in fact not bubbling up to the
window.  Not sure where the extra event comes from, though.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible patch

This fixes the testcase in this bug and in bug 155188, by just syncing
nsXULElement to nsGenericElement (again, dammit).

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 134745 [details] [diff] [review]
Possible patch

bryner?  jst?  What do you think?  I have to say I'm not too happy with this
fix... I'd prefer to introduce a NS_EVENT_FLAG_CANT_CAPTURE and set that as
well as NS_EVENT_FLAG_CANT_BUBBLE on all these load/error events.  Then we can
just check for those flags here and not have to know about event types... Right
now it looks like CANT_BUBBLE events do in fact bubble and just don't get
processed during said bubble (I could be wrong here, though).

Oh, and as for the event with no target, that's the result of the code
athttp://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#883 (
 GlobalWindowImpl::HandleDOMEvent).  It creates an nsEvent (which hence has no
target) and which also has no prescontext (so GetTarget always returns null).

Comment 12

[[e-mail]]

Comment on attachment 134745 [details] [diff] [review]
Possible patch

Note that NS_IMAGE_LOAD is already "handled" at line 3057 (should the other
types go there too?) while the tabbed browser currently requires NS_IMAGE_ERROR
to bubble.

Comment 13

[Boris Zbarsky [:bzbarsky]]

In fact, error events do bubble per the DOM spec.  Why don't we bubble them up?
 Is it to prevent window.onerror from firing for broken images?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Although, the spec talks about "error" as applying to script execution only...
too bad that's so out of touch with th reality of the past many years.  :(

Comment 15

[[e-mail]]

Comment on attachment 134745 [details] [diff] [review]
Possible patch

I'm really confused now. I think this patch stops error events from bubbling,
which is not what I think you want. As for image load events, they can't bubble
or be captured (as per my previous reference) but I don't know what you want
there.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Neil, at the moment image error events bubble in XUL (and stuff depends on
that).  They do NOT bubble in non-XUL (and stuff may depend on that too).  We
need to resolve that issue.

I think we all agree this patch is not what we want in the tree.  It was just a
demonstration of where the problem lies.  The question is what behavior we _do_
want.

My suggestion is as follows:

1)  Load events should capture but not bubble.  This requires fixing whatever
    chrome capturing event listeners we have to stop sucking and check the
    target (last time I tried making this change Tp skyrocketed).  Even so, Tp
    may suffer due to added event propagation for all those images in webpages.
    Perhaps load events should not capture up the tree either, and we should fix
    chrome handlers that depend on them to? (I doubt we have many, or any, since
    they don't capture up the tree right now.)
2)  Error events should capture and bubble.
3)  Whether events capture/bubble should be controlled by flags per comment 11,
    so that the decision on whether it captures/bubbles or not only has to be
    made once.

Comment 17

[Johnny Stenback (:jst)]

bz, I'm all for introducing an NS_EVENT_FLAG_CANT_CAPTURE flag and eliminating
the event type checking in all the places where we can. I'd much prefer that
over the patch you attached... :-)

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 134745 [details] [diff] [review]
Possible patch

OK. Should we also change the behavior of the DONT_BUBBLE flag to actually skip
bubbling instead of just preventing handling while bubbling?

Comment 19

[Johnny Stenback (:jst)]

I don't think we want to change how the bubbling works (bryner probably knows
more about this than I do tho), since IIRC there's code that cares about those
events during the bubbling stage (even if no registerd handlers are called) up
the tree, or in chrome, or somewhere.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Erk.  Could we fix that or something?  We should really not be having code that
cares about bubbling events that don't bubble....

Comment 21

[Johnny Stenback (:jst)]

Yeah, of course, assuming there's no good reason for such code (and assuming I'm
not dreaming things up here), but shouldn't the bubbling change be a separate bug?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Well... this bug is about the fact that "onload" events bubble in XUL.  They
should not be doing that.  So no, the bubbling change is exactly this bug.  The
capture change is what's sort of 'extra'.

Comment 23

[Johnny Stenback (:jst)]

Um, right. Then yeah, that should be fixed here :-)

Comment 24

[Boris Zbarsky [:bzbarsky]]

Perhaps the right thing to do is to make the DONT_BUBBLE flag work as it does
now (bubble up, but don't get processed) and set it on the relevant events...

Then consider changing how the flag works in 1.7a?  bryner?  Thoughts?

Comment 25

[Justin Turner Arthur]

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

Comment 26

[Justin Turner Arthur]

Confirmed in 1.7b under Linux, OS should be changed to "All".

Comment 27

[Cédric Chantepie]

patching file `nsXULElement.cpp'
Assertion failed: hunk, file patch.c, line 321

abnormal program termination

Comment 28

[Sean McMurray]

Confirmed to still exist in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7)
Gecko/20040616

Comment 29

[steve galatis]

(In reply to comment #28)
> Confirmed to still exist in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7)
> Gecko/20040616

Same here, with FireFox 1.0 on XP. My problem is a bit different tho, the onload
is triggered when I load content into an iframe...possibly related to the same bug.

Comment 30

[steve galatis]

(In reply to comment #29)
> (In reply to comment #28)
> > Confirmed to still exist in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7)
> > Gecko/20040616
> 
> Same here, with FireFox 1.0 on XP. My problem is a bit different tho, the onload
> is triggered when I load content into an iframe...possibly related to the same
bug.
> 
I managed a quick workaround to make it work for now in my project. Just
setAttribute the onload to 'null' after the first onload (assuming its calling a
js function), that keeps the window onload from triggering every single time I
try loading content in an iframe in the same window.

Comment 31

[Cédric Chantepie]

Attachment: Adapted patch for trunk

With that patch using the testcase the "window" alert is displayed only once.

Comment 32

[Johnny Stenback (:jst)]

Comment on attachment 172627 [details] [diff] [review]
Adapted patch for trunk

r- See comment #16.

Comment 33

[Francesco Barresi]

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

Comment 34

[Clayton Smith]

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

Comment 35

[chris hofmann]

no approved patch -> moving to beta3 nomination

Comment 36

[timeless]

please contact certain vendors which are known to heavily rely on addEventListener('load',object,true) before you change the api such that all their code breaks permanently.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

This should be now fixed (bug 234455), at least the test case wfm.
Please reopen if there is still something to fix here.