382113 - Load events aren't always dispatched to <object> element

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

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

Attachment: testcase for png and html

The load event is dispatched to the <object> element if the content is
an image, but not when the content is for example an html document.
Not yet sure when this regressed in the trunk.

Comment 1

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

Regression range 2005121705-2005121805
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-12-17+01%3A00%3A00&maxdate=2005-12-18+07%3A00%3A00&cvsroot=%2Fcvsroot
Maybe bug 309525?

Comment 2

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

I removed the patch for bug 309525 manually and it indeed did cause this regression.

Comment 3

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

Attachment: load event isn't dispatched to the contentdocument of the <object>

Things are even more broken

Comment 4

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

Comment on attachment 268216 [details]
load event isn't dispatched to the contentdocument of the <object>

When loading this testcase, there should be two green PASSes and an alert "object load".
The alert is created when load event is dispatched in the html document which is used in the <object>.

Comment 5

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

and that means probably that DocumentViewerImpl::LoadComplete
doesn't get called, nor nsDocShell::EndPageLoad or 
nsWebShell::EndPageLoad.
And there isn't busy cursor when object is loading (nsDocShell::OnStateChange doesn't get called, I think).

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Biesi: Got any ideas, this is a regression from your bug 309525

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

did comment 3's testcase work pre-bug 309525 too?

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

I'm told it did work.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

So, the reason this used to work is http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#5775

But now URILoader sets the docshell's loadgroup, so that the code there doesn't run. so, let's set this flag manually.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 268839 [details] [diff] [review]
patch

Looks good, but could we document on nsIURILoader's API stuff that it doesn't do that?

And please make sure to check in a regression test along with the patch.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #10)
> (From update of attachment 268839 [details] [diff] [review])
> Looks good, but could we document on nsIURILoader's API stuff that it doesn't
> do that?

Do you think that's necessary? URI Loader never sets that flag (and never checks it either, although nsDocLoader does)

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

cc'ing bz so that he actually sees the question in the previous comment...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Oh, hmm.  And in this case we're missing the relevant part of docloader?

Might still want to document on nsIURILoader, then...

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

No, the flag is set by docshell normally (DoChannelLoad). But we start our load outside of docshell, so that code isn't run.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: testcase

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

(the testcase is basically Smaug's second testcase mochitestified, without the PNG part)

Comment 17

[Boris Zbarsky [:bzbarsky]]

Oh, I see.  Yeah, no need for the comment then.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 269001 [details] [diff] [review]
testcase

>+++ b/content/base/test/test_bug382113.html
>+  ok(childGotOnload, "Child got load event");
>+  ok(objectGotOnload, "Object got load event");

I'd use |is(*GotOnload, true, "...")| for both of these.  That will give a clearer error message if it fails.

r=me with that.

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch for checkin

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in content/base/src/nsObjectLoadingContent.cpp;
/cvsroot/mozilla/content/base/src/nsObjectLoadingContent.cpp,v  <--  nsObjectLoadingContent.cpp
new revision: 1.43; previous revision: 1.42
done
Checking in content/base/test/Makefile.in;
/cvsroot/mozilla/content/base/test/Makefile.in,v  <--  Makefile.in
new revision: 1.14; previous revision: 1.13
done
RCS file: /cvsroot/mozilla/content/base/test/bug382113_object.html,v
done
Checking in content/base/test/bug382113_object.html;
/cvsroot/mozilla/content/base/test/bug382113_object.html,v  <--  bug382113_object.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/content/base/test/test_bug382113.html,v
done
Checking in content/base/test/test_bug382113.html;
/cvsroot/mozilla/content/base/test/test_bug382113.html,v  <--  test_bug382113.html
initial revision: 1.1
done

Comment 21

[Jeff Walden [:Waldo]]

None of the tinderboxen took very well to this patch, and the test included in the patch fails on at least one, probably all of them.

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

mac and windows didn't took well, because the test fails there. I have a patch, attaching it in a second.

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: additional patch

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

I changed the is to todo now in the testcase so that it can pass until the test is checked in.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 269515 [details] [diff] [review]
additional patch

Hmmm...  Do we need a similar fix in other places where we move things between loadgroups?  nsJSChannel::EvaluateScript comes to mind, though there we might be saved by the onload-blocking patch I wrote elsewhere.  What about other callsites?

In any case, I'd think we would want to do the add/remove only if the loadgroups differ.  Otherwise it looks like the channel will end up wholly outside a loadgroup, no?

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

I copied this from one of the other callsites in fact (the one in docshell). I can't see any other callers; where does EvaluateScript do this kind of thing? It seems to only remove the channel from its load group, not add it to another one.

But good point about doing this only when loadgroups differ.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> where does EvaluateScript do this kind of thing?

It removes the javascript: channel, then AsyncOpen()s the string channel.  The effect is the same: onload can fire in between.  Except I guess the first channel is a LOAD_BACKGROUND, so it's not an issue anyway.  OK.  So just fix the same-loadgroup case?

Comment 28

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: additional patch v2

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 272719 [details] [diff] [review]
additional patch v2

>Index: uriloader/base/nsURILoader.cpp

No, this seems wrong.  If the request is already in the right loadgroup, we'll basically call AddRequest() twice o the same request, and only remove it once.  That will at the very least confuse the loadgroup's mForegroundCount.

In fact, I wish we could make the loadgroup assert when that happens.

I think you want that SameCOMIdentity check around all this code, basically.

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

oh, hmm. Yeah, except that I also have to deal with a potentially-null channel loadgroup. but I'll deal.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Note that SameCOMIdentity is null-safe...

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

yeah, but in the null case I have to call addRequest but not removeRequest.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Sure.

Comment 34

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: additional patch v3

only do anything if loadgroups differ. make addRequest assert if the request is already in the loadgroup.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 272851 [details] [diff] [review]
additional patch v3

Very nice!

Comment 36

[Christian :Biesinger (don't email me, ping me on IRC)]

Additional patch v3 checked in

Checking in content/base/test/test_bug382113.html;
/cvsroot/mozilla/content/base/test/test_bug382113.html,v  <--  test_bug382113.html
new revision: 1.4; previous revision: 1.3
done
Checking in netwerk/base/src/nsLoadGroup.cpp;
/cvsroot/mozilla/netwerk/base/src/nsLoadGroup.cpp,v  <--  nsLoadGroup.cpp
new revision: 1.73; previous revision: 1.72
done
Checking in uriloader/base/nsURILoader.cpp;
/cvsroot/mozilla/uriloader/base/nsURILoader.cpp,v  <--  nsURILoader.cpp
new revision: 1.144; previous revision: 1.143
done