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.

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?

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

Things are even more broken

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>.

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).

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

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

I'm told it did work.

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 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.

(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)

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

Oh, hmm. And in this case we're missing the relevant part of docloader? Might still want to document on nsIURILoader, then...

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

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

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

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.

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

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.

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

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

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?

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.

> 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 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.

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

Note that SameCOMIdentity is null-safe...

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

Sure.

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

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

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