Closed Bug 783589 Opened 13 years ago Closed 12 years ago

Intermittent test_popup-navigates-children.html, test_sibling-matching-parent.html | Should be able to navigate on-domain opener's children (or sibling with on-domain parent) etc.

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla26
Tracking Flags:
Tracking Status
firefox24 --- fixed
firefox25 --- fixed
firefox26 --- fixed

People

(Reporter: emorley, Assigned: mayhemer)

References

(
URL
)

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 2 obsolete files)

v2 [as landed]
1.41 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
Reenable the test
1.29 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
Now that m3 actually runs all the way through... :-) Android Tegra 250 mozilla-inbound opt test mochitest-3 on 2012-08-17 08:31:53 PDT for push 314b3a20a88c slave: tegra-212 https://tbpl.mozilla.org/php/getParsedLog.php?id=14471057&tree=Mozilla-Inbound { 19 INFO TEST-START | /tests/docshell/test/navigation/test_popup-navigates-children.html 20 INFO TEST-PASS | /tests/docshell/test/navigation/test_popup-navigates-children.html | Should be able to navigate on-domain opener's children by setting location. - This frame was navigated. should equal This frame was navigated. 21 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/navigation/test_popup-navigates-children.html | Should be able to navigate on-domain opener's children by calling window.open. - got , expected This frame was navigated. 22 INFO TEST-PASS | /tests/docshell/test/navigation/test_popup-navigates-children.html | Should be able to navigate on-domain opener's children by submitting form. - This frame was navigated. should equal This frame was navigated. 23 INFO TEST-PASS | /tests/docshell/test/navigation/test_popup-navigates-children.html | Should be able to navigate on-domain opener's children by targeted hyperlink. - This frame was navigated. should equal This frame was navigated. 24 INFO TEST-END | /tests/docshell/test/navigation/test_popup-navigates-children.html | finished in 4860ms }
Summary: Intermittent Android test_popup-navigates-children.html | Should be able to navigate on-domain opener's children by calling window.open. - got , expected This frame was navigated. → Intermittent Android test_popup-navigates-children.html | Should be able to navigate on-domain opener's children by calling window.open (or submitting form). - got , expected This frame was navigated.
https://tbpl.mozilla.org/php/getParsedLog.php?id=15795962&tree=Mozilla-Inbound 51611 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/navigation/test_sibling-matching-parent.html | Should be able to navigate sibling with on-domain parent by targeted hyperlink. - got TypeError: SpecialPowers.wrap(...).document.body is null, expected This frame was navigated.
Summary: Intermittent Android test_popup-navigates-children.html | Should be able to navigate on-domain opener's children by calling window.open (or submitting form). - got , expected This frame was navigated. → Intermittent Android test_popup-navigates-children.html, test_sibling-matching-parent.html | Should be able to navigate on-domain opener's children (or sibling with on-domain parent) etc.
I starred that WinXP debug as this Android bug because that's the slowest thing we've got which isn't Android, lending a bit of credence to the idea that this is just a race (between a tortoise and a lettuce leaf maybe, but still a race).
OS: Android → All
Hardware: ARM → All
Summary: Intermittent Android test_popup-navigates-children.html, test_sibling-matching-parent.html | Should be able to navigate on-domain opener's children (or sibling with on-domain parent) etc. → Intermittent test_popup-navigates-children.html, test_sibling-matching-parent.html | Should be able to navigate on-domain opener's children (or sibling with on-domain parent) etc.
Whiteboard: [orange]
Too many failures; no one looking at this -> test disabled on Android: https://hg.mozilla.org/integration/mozilla-inbound/rev/da87be86c167
Whiteboard: [test disabled on Android][leave open]
This is a little modified version of the testcase, that works fine, but causes failures on Android. http://people.mozilla.org/~mwargers/mochitestjs2/test_popup-navigates-children.html Also notice how the contents of the iframes are sometimes filled in, but most of the time partly (sometimes 1, sometimes 2, sometimes 3). Iirc, in Fennec background tabs get their timers suspended. Maybe that has something to do with this? The test could perhaps be rewritten to only open 1 window at a time, that might help fix this issue too.
Flaw in the test is much more visible when running it under the new cache backend with a pref switched to use the old cache (the old cache is wrapped by the new cache API in that case) [2]. The problem is in xpcWaitForFinishedFrames [1]. I added some logs to searchForFinishedFrames() sub-function before frameFinished() is called and clearly it's called much more often then expected 4 times for frames being modified (it behaves the same way when running on mozilla central unmodified code). When I change the number of expected frames to load from 4 to 8, the test works as expected (surprisingly doesn't hang). [1] http://hg.mozilla.org/mozilla-central/annotate/e42dce3209da/docshell/test/navigation/NavigationUtils.js#l154 [2] https://tbpl.mozilla.org/?tree=Gum&rev=62b177577bbc (mochitest-2)
OK, the problem is at: 171 function contains(obj, arr) { 172 for (var i = 0; i < arr.length; i++) { 173 if (obj === arr[i]) <<<<<<< 174 return true; 175 } 176 return false; 177 } The compare never passes, so: 187 if (!contains(win, finishedWindows)) { 188 finishedWindows.push(win); 189 frameFinished(); 190 } Passes for every call of searchForFinishedFrames (called in intervals). I've added a log: if (!contains(win, finishedWindows)) { finishedWindows.push(win); dump("************** win info: win.location="+win.location+" win.document.body.textContent="+win.document.body.textContent+" name="+win.name+"\n"); frameFinished(); } And I'm getting: 0:23.60 ************** win info: win.location=data:text/html,<html><body>This%20frame%20was%20navigated.</body></html> win.document.body.textContent=This fra me was navigated. name=child3 0:24.26 ************** win info: win.location=data:text/html,<html><body>This%20frame%20was%20navigated.</body></html> win.document.body.textContent=This fra me was navigated. name=child3 0:24.86 ************** win info: win.location=data:text/html,<html><body>This%20frame%20was%20navigated.</body></html> win.document.body.textContent=This fra me was navigated. name=child2 0:24.91 ************** win info: win.location=data:text/html,<html><body>This%20frame%20was%20navigated.</body></html> win.document.body.textContent=This fra me was navigated. name=child3 It 4 times passes for child3 and child2, sooner then child0 and 1 have a chance to update. The question is: how should we correctly compare window objects added to the finishedWindows array? I'm playing with storing a windows's name property. It seems like most of the tests using xpcWaitForFinishedFrames give names to all involved windows. So it should work.
Attached patch v1 (obsolete) — — Splinter Review
- see comment 410 or read the short version: - finishedWindows array collects windows we have found already navigated (via value of document.body) - it keeps references to those window objects - however, comparing newly found window objects with the previously stored in the array with === gives always false (the objects always differ) - fix: the finishedWindows array collects window.name property - fixed test_not-opener.html (reverted [1]) [1] http://hg.mozilla.org/mozilla-central/diff/f85e9589bd11/docshell/test/navigation/test_not-opener.html
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #796003 - Flags: review?(jonas)
Comment on attachment 796003 [details] [diff] [review] v1 Review of attachment 796003 [details] [diff] [review]: ----------------------------------------------------------------- My review list is far too long right now. And I'm not a docshell peer.
Attachment #796003 - Flags: review?(jonas) → review?
Attachment #796003 - Flags: review? → review?(justin.lebar+bug)
window.name isn't unique and is often "", afaict. Given that we're making this change to a test-utils script, we should try to write something more robust. > - however, comparing newly found window objects with the previously stored in the array with === > gives always false (the objects always differ) Are you sure this doesn't just mean that they're different window objects? The window object will change when you navigate. I think what this code wants (maybe?) is the outer-window ID. You can get this using special powers: SpecialPowers.getDOMWindowUtils(win).outerWindowID An outer window ID essentially identifies the frame that the window lives in, while the inner window ID changes every time the page navigates.
Attachment #796003 - Flags: review?(justin.lebar+bug)
(In reply to Justin Lebar [:jlebar] from comment #414) > I think what this code wants (maybe?) is the outer-window ID. You can get > this using special powers: > > SpecialPowers.getDOMWindowUtils(win).outerWindowID > Exception: Trying to double-wrap object! so I tried to do directly to win.outerWindowID and it doesn't work either - outerWindowID is undefined. So I'm stuck. Anyway, my patch was wrong since on Android only it broke docshell/test/navigation/test_not-opener.html with "Should not be able to navigate popup's popup by targeted hyperlink. - got 1, expected 2" that became a regular failure: https://tbpl.mozilla.org/?tree=Gum&rev=45606aa5ea5c
And to explain what the utility function does: It loops in 500ms intervals over all open windows and examines their document.body content. When it is found to be the string expected AFTER the window had been navigated, it's stored to the array as "OK, we have this window now, it has been navigated". This way we add to the array every window we find it has been navigated, but we prevent adding the same window twice. When length of the array == number of expected windows to navigate, it finishes the test (calls the callback).
> So I'm stuck. Looks like you figured it out?
(In reply to Justin Lebar [:jlebar] from comment #419) > > So I'm stuck. > > Looks like you figured it out? Try is in progress. Locally on win the tests are passing on m-c, gum with old cache backend and gum with the new cache backend. Depends only on how android tests pass.
Attached patch v2 (obsolete) — — Splinter Review
So, this works me. Thanks for help, Justin! https://tbpl.mozilla.org/?tree=Try&rev=76bc58c09da6 https://tbpl.mozilla.org/?tree=Try&rev=ccf759fc64f0 and if interested: https://tbpl.mozilla.org/?tree=Gum&rev=f1e2c504a9e8
Attachment #796003 - Attachment is obsolete: true
Attachment #797435 - Flags: review?(justin.lebar+bug)
Comment on attachment 797435 [details] [diff] [review] v2 lgtm!
Attachment #797435 - Flags: review?(justin.lebar+bug) → review+
Attached patch v2 [as landed] — — Splinter Review
I've left the "6" in test_not-opener.html. Otherwise it fails on Android. Not sure why but it is proven. Try runs are with 6 anyway. https://hg.mozilla.org/mozilla-central/rev/065727546e13
Attachment #797435 - Attachment is obsolete: true
Attachment #797759 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Whiteboard: [test disabled on Android][leave open] → [test disabled on Android]
See comment 424
Flags: needinfo?(justin.lebar+bug)
(In reply to Honza Bambas (:mayhemer) from comment #424) > Hmm.. should we also back > https://hg.mozilla.org/mozilla-central/rev/da87be86c167 out? https://tbpl.mozilla.org/?tree=Try&rev=a2aed792e11c
20+ test passes on try are green so far :)
Attachment #797937 - Flags: review?(justin.lebar+bug)
Comment on attachment 797937 [details] [diff] [review] Reenable the test cool!
Attachment #797937 - Flags: review?(justin.lebar+bug) → review+
You'll need to enable it in androidx86.json as well.
I retriggered them a *few* more times and no failures. :) https://hg.mozilla.org/integration/mozilla-inbound/rev/7cf1f45052fa
Whiteboard: [test disabled on Android]
Flags: needinfo?(justin.lebar+bug)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #431) > https://hg.mozilla.org/mozilla-central/rev/7cf1f45052fa Thanks Ryan!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: