Closed Bug 431833 Opened 12 years ago Closed 12 years ago

"DOMFrameContentLoaded" event isn't fired anymore

Categories

(Core :: DOM: Events, defect, major)

x86
All
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: pointervoid, Assigned: bzbarsky)

References

Details

(Keywords: regression, testcase, Whiteboard: [has patch][needs review jst])

Attachments

(3 files, 1 obsolete file)

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.
Attached file Testcase. Unzip and open in FF3 beta 5 (obsolete) —
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
Attached file standalone testcase
In Firefox 2.0.0.14, this shows:
PASS: f1
PASS: f2
PASS: f3

On current trunk, is shows "FAIL".
Attachment #319004 - Attachment is obsolete: true
Flags: blocking1.9?
Summary: Event "DOMFrameContentLoaded" don't fired at all → "DOMFrameContentLoaded" event isn't fired anymore
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.
Attachment #319112 - Flags: superreview?(jst)
Attachment #319112 - Flags: review?(bent.mozilla)
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.
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...
JST/BZ do we need this for sure for FF3 RC?
Comment on attachment 319112 [details] [diff] [review]
This seems to fix it

Ah, good deal.
Attachment #319112 - Flags: review?(bent.mozilla) → review+
(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.
Flags: blocking1.9? → blocking1.9+
Keywords: regression
Whiteboard: [has patch][needs review jst]
Assignee: nobody → bzbarsky
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 :)
Attachment #319112 - Flags: superreview?(jst)
Attachment #319112 - Flags: superreview+
Attachment #319112 - Flags: approval1.9?
Comment on attachment 319112 [details] [diff] [review]
This seems to fix it

a1.9=beltzner
Attachment #319112 - Flags: approval1.9? → approval1.9+
For what it's worth, I think risk is low.  ;)

Checked in.
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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.
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.
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.
In fact, when I run it locally in my Linux opt build it fails every time.
Not only can the events fire before the listener is registered, but they could fire out of order too.
I just pushed a1cf0f78f9da to disable the test.
Attached patch fix testSplinter Review
This passes on my Linux build and it looks sound to me.

Boris, does this test what needs to be tested?
Attachment #329641 - Flags: review?(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.
Attachment #329641 - Flags: review?(bzbarsky) → review+
Pushed roc's patch as e58e7f4c699a
Flags: in-testsuite+
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
You need to log in before you can comment on or make changes to this bug.