Closed Bug 382113 Opened 13 years ago Closed 12 years ago

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

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: Biesinger)

References

Details

(Keywords: regression)

Attachments

(4 files, 4 obsolete files)

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.
I removed the patch for bug 309525 manually and it indeed did cause this regression.
Blocks: 309525
Flags: blocking1.9?
Assignee: nobody → Olli.Pettay
Flags: blocking1.9? → blocking1.9+
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.
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: Olli.Pettay → cbiesinger
Status: NEW → ASSIGNED
Attachment #268839 - Flags: superreview?(bzbarsky)
Attachment #268839 - Flags: review?(bzbarsky)
OS: Linux → All
Hardware: PC → All
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.
Attachment #268839 - Flags: superreview?(bzbarsky)
Attachment #268839 - Flags: superreview+
Attachment #268839 - Flags: review?(bzbarsky)
Attachment #268839 - Flags: review+
(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.
Attached patch testcase (obsolete) — Splinter Review
Attachment #269001 - Flags: review?(bzbarsky)
(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.
Attachment #269001 - Flags: review?(bzbarsky) → review+
Attachment #268839 - Attachment is obsolete: true
Attachment #269001 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch additional patch (obsolete) — Splinter Review
Attachment #269515 - Flags: superreview?(bzbarsky)
Attachment #269515 - Flags: review?(bzbarsky)
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?
Attachment #269515 - Flags: superreview?(bzbarsky)
Attachment #269515 - Flags: superreview-
Attachment #269515 - Flags: review?(bzbarsky)
Attachment #269515 - Flags: review-
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?
Attached patch additional patch v2 (obsolete) — Splinter Review
Attachment #269515 - Attachment is obsolete: true
Attachment #272719 - Flags: superreview?(bzbarsky)
Attachment #272719 - Flags: review?(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.
Attachment #272719 - Flags: superreview?(bzbarsky)
Attachment #272719 - Flags: superreview-
Attachment #272719 - Flags: review?(bzbarsky)
Attachment #272719 - Flags: review-
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.
only do anything if loadgroups differ. make addRequest assert if the request is already in the loadgroup.
Attachment #272719 - Attachment is obsolete: true
Attachment #272851 - Flags: superreview?(bzbarsky)
Attachment #272851 - Flags: review?(bzbarsky)
Comment on attachment 272851 [details] [diff] [review]
additional patch v3

Very nice!
Attachment #272851 - Flags: superreview?(bzbarsky)
Attachment #272851 - Flags: superreview+
Attachment #272851 - Flags: review?(bzbarsky)
Attachment #272851 - Flags: review+
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

Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Depends on: 453969
Depends on: 468204
No longer depends on: 468204
You need to log in before you can comment on or make changes to this bug.