431833 - "DOMFrameContentLoaded" event isn't fired anymore

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[PVOID]

User-Agent:       Opera/9.24 (Windows NT 5.1; U; ru)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5

In firefox2 event DOMFrameContentLoaded fired when frame content dom loaded, in all firefox3 betas not.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[PVOID]

Attachment: Testcase. Unzip and open in FF3 beta 5

Comment 2

[Ria Klaassen (not reading all bugmail)]

Regression range is 
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1202506740&maxdate=1202508779
Caused by Bug 409390 ?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: standalone testcase

In Firefox 2.0.0.14, this shows:
PASS: f1
PASS: f2
PASS: f3

On current trunk, is shows "FAIL".

Comment 4

[Boris Zbarsky [:bzbarsky]]

Er, yes.  Here's the problem:

+    nsIDocument* parent = mParentDocument;
+    while (parent) {
+      parent = parent->GetParentDocument();
+      if (!parent) {
         break;
       }

That skips over the immediate parent.  We should be getting the parent of the current parent where we used to: at the end of the while loop, not at the beginning.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: This seems to fix it

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 319112 [details] [diff] [review]
This seems to fix it

>     nsCOMPtr<nsIDocument> parent = mParentDocument;
>-    while (parent) {
> ...
>+    do {

Can mParentDocument ever be null? The old code seemed to guard against that but this change will not.

Comment 7

[Boris Zbarsky [:bzbarsky]]

If mParentDocument is null, target_frame will be null too, and then we never get to this code.  I guess I should have included 2 more lines of context to make that obvious...

Comment 8

[Mike Schroepfer]

JST/BZ do we need this for sure for FF3 RC?

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 319112 [details] [diff] [review]
This seems to fix it

Ah, good deal.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #8)
> JST/BZ do we need this for sure for FF3 RC?

Vlad says we do, and it's a regression, so blocking.

Comment 11

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

Comment on attachment 319112 [details] [diff] [review]
This seems to fix it

I'll let bz comment on the risk here, but the patch feels pretty obvious and safe to me, and it restores behavior to what we used to have.

And it has tests :)

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 319112 [details] [diff] [review]
This seems to fix it

a1.9=beltzner

Comment 13

[Boris Zbarsky [:bzbarsky]]

For what it's worth, I think risk is low.  ;)

Checked in.

Comment 14

[David Baron :dbaron:]

The test added in this bug just randomly failed on "WINNT 5.2 qm-win2k3-pgo01 dep unit test on 2008/05/28 18:47:22" on the Firefox tinderbox:

*** 2519 ERROR FAIL | DOMFrameContentLoaded events didn't fire? | got "FAIL", expected "PASS: f1. PASS: f2. PASS: f3. PASS: f4. " | /tests/content/base/test/test_bug431833.html

Looking at the test -- what guarantees that the DOMFrameContentLoaded events are fired *after* the listener for them is added?  It seems like they could fire before.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Hmmm....  I recall worrying about that and deciding that the loads being async was good enough, but of course it's not: if we happen to interrupt parsing after we parse the <iframe> tags we might end up spinning the event loop, etc.

I wouldn't think that should happen, but the parser-interruption stuff is black-magic enough that it might.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This test is failing constantly on the Linux test box qm-centos5-moz2-02-hw dep unit test ... as in, today it failed for the last 8 test runs in a row. It's unfortunate since that looks like the only random orange on the box today. If this test is known to be unreliable it should be disabled.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In fact, when I run it locally in my Linux opt build it fails every time.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Not only can the events fire before the listener is registered, but they could fire out of order too.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I just pushed a1cf0f78f9da to disable the test.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix test

This passes on my Linux build and it looks sound to me.

Boris, does this test what needs to be tested?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 329641 [details] [diff] [review]
fix test

Yeah, good catch.  I think the ordering thing is probably something we _can_ depend on for now, but as we start doing more load prioritization it won't be, so good to take out that dependency.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Pushed roc's patch as e58e7f4c699a

Comment 23

[(not currently active) Ted Mielczarek]

Comment on attachment 329641 [details] [diff] [review]
fix test

I committed this test fix to CVS as well:
Checking in content/base/test/test_bug431833.html;
/cvsroot/mozilla/content/base/test/test_bug431833.html,v  <--  test_bug431833.ht
ml
new revision: 1.2; previous revision: 1.1
done