Closed Bug 765192 Opened 13 years ago Closed 12 years ago

Intermittent browser_bug343515.js | Tab 2 should have 2 iframes - Got 1, expected 2 | uncaught JS exception ... | Test timed out

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla19

People

(Reporter: emorley, Assigned: Felipe)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files)

Bug 765192 - Fix broken test. v1
6.85 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
Fix and reenable test
11.60 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
Rev3 WINNT 5.1 mozilla-inbound opt test mochitest-other on 2012-06-14 19:58:14 PDT for push 36e3e2913c3d slave: talos-r3-xp-005 https://tbpl.mozilla.org/php/getParsedLog.php?id=12683765&tree=Mozilla-Inbound { TEST-PASS | chrome://mochitests/content/browser/docshell/test/navigation/browser_bug343515.js | Tab 2 should be inactive TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/docshell/test/navigation/browser_bug343515.js | Tab 2 should have 2 iframes - Got 1, expected 2 Stack trace: JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 428 JS frame :: chrome://mochitests/content/browser/docshell/test/navigation/browser_bug343515.js :: step4 :: line 134 JS frame :: chrome://mochitests/content/browser/docshell/test/navigation/browser_bug343515.js :: frameLoadCallback :: line 56 JS frame :: chrome://mochitests/content/browser/docshell/test/navigation/browser_bug343515.js :: frameLoadCallback :: line 49 JS frame :: chrome://mochitests/content/browser/docshell/test/navigation/browser_bug343515.js :: frameLoadCallback :: line 49 JS frame :: chrome://mochikit/content/browser-test.js :: <TOP_LEVEL> :: line 459 TEST-PASS | chrome://mochitests/content/browser/docshell/test/navigation/browser_bug343515.js | Tab 2 iframe 0 should have 1 iframes TEST-PASS | chrome://mochitests/content/browser/docshell/test/navigation/browser_bug343515.js | Tab2 iframe 0 should be inactive TEST-PASS | chrome://mochitests/content/browser/docshell/test/navigation/browser_bug343515.js | Tab2 iframe 0 subiframe 0 should be inactive TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/docshell/test/navigation/browser_bug343515.js | an unexpected uncaught JS exception reported through window.onerror - TypeError: aWindow is undefined at chrome://mochitests/content/browser/docshell/test/navigation/browser_bug343515.js:9 Stack trace: JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 994 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 TEST-INFO | chrome://mochitests/content/browser/docshell/test/navigation/browser_bug343515.js | Console message: [JavaScript Error: "TypeError: aWindow is undefined" {file: "chrome://mochitests/content/browser/docshell/test/navigation/browser_bug343515.js" line: 9}] TEST-INFO | chrome://mochitests/content/browser/docshell/test/navigation/browser_bug343515.js | Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must to be declared in the document or in the transfer protocol." {file: "http://mochi.test:8888/browser/docshell/test/navigation/bug343515_pg3.html" line: 0}] TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/docshell/test/navigation/browser_bug343515.js | Test timed out args: ['c:\\talos-slave\\test\\build\\bin\\screenshot.exe', 'c:\\docume~1\\cltbld\\locals~1\\temp\\mozilla-test-fail_mt3uve'] SCREENSHOT: <snip> INFO TEST-END | chrome://mochitests/content/browser/docshell/test/navigation/browser_bug343515.js | finished in 30032ms TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/docshell/test/navigation/browser_bug343515.js | Found a tab after previous test timed out: http://mochi.test:8888/browser/docshell/test/navigation/bug343515_pg3.html TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/docshell/test/navigation/browser_bug343515.js | Found a tab after previous test timed out: http://mochi.test:8888/browser/docshell/test/navigation/bug343515_pg1.html }
I don't really recall why I'm CCed on this bug, but it's not in any code that I work on, so I'm unlikely to ever do anything about it. :-(
Ed points out that I actually wrote this test long ago. :-)
This never got full review - see bug 343515#c45. Naughty naughty Dolske. ;-) There were a couple of things wrong here: 1 - The whole recursive frame load waiting thing was totally unnecessary. Load doesn't fire on the window until all descendant iframes have loaded. 2 - The chrome event handler wasn't filtering by target, which it should. This is probably to blame for the intermittent orange. 3 - Setting window.location during document load actually invokes window.location.replace. 4 - Session history navigation seems to have weird behavior when called from an onload handler. I wanted to investigate this further, but I've already spent too much time on this. SimpleTest.executeSoon() seems to do the trick here.
Attachment #677736 - Flags: review?(dolske)
Comment on attachment 677736 [details] [diff] [review] Bug 765192 - Fix broken test. v1 Review of attachment 677736 [details] [diff] [review]: ----------------------------------------------------------------- I don't see anything obviously terrible. Ahem.
Attachment #677736 - Flags: review?(dolske) → review+
Hrmph, well I guess that suggests this didn't fix it? Blarg.
https://hg.mozilla.org/mozilla-central/rev/6fec70b5b508
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 813242
Since the issue has morhped, filed bug 813242 for the new one; closing this.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Assignee: nobody → bobbyholley+bmo
Whiteboard: [orange]
I may need to back this out to fix Bug 825544.
(In reply to Olli Pettay [:smaug] from comment #105) > I may need to back this out to fix Bug 825544. This is only a test change to make behavior more defined. Rather than backing it out, can you just update the test so that it passes given whatever navigation behavior you decide?
I need to backout so much in order to fix Bug 825544, that I really don't want to make any changes. (Bug 825544 needs to be fixed in beta too.)
Also, why does this need to be backed out? The primary relevant to that bug is that this test _avoids_ navigating via window.location because of the weirdness of the bug in question. So it seems like this test should make things agnostic to the bug you're backing out.
because the backout will bring back the old behavior where .setAttribute("src", href) has the same behavior as location = href
(In reply to Olli Pettay [:smaug] from comment #109) > because the backout will bring back the old behavior where > .setAttribute("src", href) has the > same behavior as location = href Oh, I see. I thought that bug only affected nsLocation. My bad.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [disable-me 2013-04-01]
smaug/bholley, any idea what might have caused this to spike so significantly over the last month or so?
Flags: needinfo?(bugs)
Flags: needinfo?(bobbyholley+bmo)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #279) > smaug/bholley, any idea what might have caused this to spike so > significantly over the last month or so? Not offhand, no. I tried my darndest to fix this back in November, so if it's to go anywhere maybe someone else should look at it. ;-)
Flags: needinfo?(bobbyholley+bmo)
Just a heads up to imagelib and memshrink folks - this test is in jeopardy of being disabled, and AFAIK it's the only thing that tests the isActive API on docshells, which we use to tune a bunch of imagelib heuristics (locking, etc). I took a crack at fixing the orange, but it made it worse, and then my change got backed out anyway for other reasons. This probably needs love from someone who understands browser-chrome stuff better than I do.
Assignee: bobbyholley+bmo → nobody
(In reply to Bobby Holley (:bholley) from comment #316) > Just a heads up to imagelib and memshrink folks - this test is in jeopardy > of being disabled, and AFAIK it's the only thing that tests the isActive API > on docshells, which we use to tune a bunch of imagelib heuristics (locking, > etc). > > I took a crack at fixing the orange, but it made it worse, and then my > change got backed out anyway for other reasons. This probably needs love > from someone who understands browser-chrome stuff better than I do.
Flags: needinfo?(joe)
Have disabled on OS X (was already disabled on Windows in bug 813242) for now: http://hg.mozilla.org/integration/mozilla-inbound/rev/1998eb59e369
Whiteboard: [disable-me 2013-04-01] → [test disabled on Windows & OS X][leave open]
Perhaps Seth has some idea
Flags: needinfo?(joe) → needinfo?(seth)
(In reply to Joe Drew (:JOEDREW! \o/) from comment #390) > Perhaps Seth has some idea Well, IMO, somebody with a deep understanding of browser-chrome needs to go through this test and fix the things that it does wrong. I'm just saying that this test is primarily useful to imagelib, and thus imagelib folks would be the most interested in persuading someone on fx-team to look at it.
Yeah, I'm probably not the person who can get this resolved the quickest. Gavin, can you suggest a person who'd be familiar with the relevant code?
Flags: needinfo?(seth) → needinfo?(gavin.sharp)
Felipe, could you look into this (and bug 813242)?
Flags: needinfo?(gavin.sharp) → needinfo?(felipc)
Yeah, i'm giving it a try. But I can't reproduce it locally, so it's been shooting in the dark. I pushed some debugging code to the tree to get more info from the failures.. https://hg.mozilla.org/integration/mozilla-inbound/rev/73316004a049 ..and then I remember that the test is disabled on trunk.. sry for the unecessary push I'll be sending it to try though to give various runs and see if I can get the data from there
Assignee: nobody → felipc
Flags: needinfo?(felipc)
Finally got one failure on try that was very helpful. The frameLoadWaiter function smells very fishy and I believe it can fail in some edge cases, depending on the order that the iframes finish loading. I pushed to try a patch with a much simpler and well-behaved function to replace it, let's see how it goes: https://tbpl.mozilla.org/?tree=Try&rev=83dd6e22a3db
Hard to say for sure, but the try run looks encouraging! https://tbpl.mozilla.org/?tree=Try&rev=83dd6e22a3db Who wants to review? Gavin? The test needs to wait for all the subframes to have loaded before proceeding to the next step. Since iframe loading is async, it's not enough to listen for "load" on the top document. To do this we now wait the explicit number of loads that we know should happen, instead of using a function that iterates through the documents trying to see what frames are loaded or not.
Attachment #748570 - Flags: review?(gavin.sharp)
Comment on attachment 748570 [details] [diff] [review] Fix and reenable test (In reply to :Felipe Gomes from comment #403) > The test needs to wait for all the subframes to have loaded before > proceeding to the next step. Since iframe loading is async, it's not enough > to listen for "load" on the top document. So 1) in comment 86 was wrong, and that's what caused that attempted test fix to fail? This fix looks good, thanks for chasing this one down. It would be helpful for future test hackers/readers to add comments explaining how you got to the "n" values for the various calls to nShotsListener (e.g. "bug343515_pg3.html has 2 iframes, one of which contains a sub-iframe, so we need to wait for a total of 4 load events (the page itself and each descendent frame)").
Attachment #748570 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/31e307e5d24c
Flags: needinfo?(bugs)
Whiteboard: [test disabled on Windows & OS X][leave open]
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #404) > (In reply to :Felipe Gomes from comment #403) > > The test needs to wait for all the subframes to have loaded before > > proceeding to the next step. Since iframe loading is async, it's not enough > > to listen for "load" on the top document. > > So 1) in comment 86 was wrong, and that's what caused that attempted test > fix to fail? I think that might still be correct and I was mistakenly speaking from memory (thinking about the about:blank case where that's true). But since that approach was already attempted and didn't work (granted, with other changes together), I decided to go on a different route to fix it. As it seems to have worked, I didn't go back and check.
Smaug, can you enlighten us as to the onload behavior under discussion here?
Flags: needinfo?(bugs)
load event for top level document is fired after all the child documents have been loaded. But of course if we end up starting load of an iframe after the top level load, then perhaps we need to check explicitly whether all the iframes have been loaded.
Flags: needinfo?(bugs)
https://hg.mozilla.org/mozilla-central/rev/31e307e5d24c
Status: REOPENED → RESOLVED
Closed: 13 years ago → 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: