Closed Bug 857427 Opened 13 years ago Closed 12 years ago

Intermittent browser_save_link-perwindowpb.js | Test timed out | Found a browser window after previous test timed out

Categories

(Firefox :: Private Browsing, defect)

Product:
Firefox
Component:
Private Browsing
Platform:
x86_64
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 30
Tracking Flags:
Tracking Status
firefox27 --- wontfix
firefox28 --- wontfix
firefox29 --- fixed
firefox30 --- fixed
firefox-esr24 --- wontfix
b2g-v1.2 --- wontfix
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: philor, Assigned: ehsan.akhgari)

References

Details

(Keywords: intermittent-failure, Whiteboard: p=0 s=it-31c-30a-29b.2 [qa-])

Attachments

(3 files, 1 obsolete file)

Fix browser_save_link_perwindow_pb
6.71 KB, patch
Details | Diff | Splinter Review
Wait for delayed startup to be finished before starting tests in a window
1.49 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
possible fix
1.63 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
https://tbpl.mozilla.org/php/getParsedLog.php?id=21370416&tree=Mozilla-Inbound Rev4 MacOSX Snow Leopard 10.6 mozilla-inbound opt test mochitest-browser-chrome on 2013-04-02 12:41:38 PDT for push 88288ea65ef8 slave: talos-r4-snow-015 19:45:35 INFO - TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_save_link-perwindowpb.js | file was read successfully 19:45:35 INFO - TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_save_link-perwindowpb.js | Console message: [JavaScript Error: "[Exception... "Component returned failure code: 0x80470002 (NS_BASE_STREAM_CLOSED) [nsIInputStream.available]" nsresult: "0x80470002 (NS_BASE_STREAM_CLOSED)" location: "JS frame :: chrome://mochitests/content/browser/browser/base/content/test/browser_save_link-perwindowpb.js :: <TOP_LEVEL> :: line 79" data: no]" {file: "chrome://mochitests/content/browser/browser/base/content/test/browser_save_link-perwindowpb.js" line: 79}] 19:46:04 WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_save_link-perwindowpb.js | Test timed out (screenshot) 19:46:08 INFO - INFO TEST-END | chrome://mochitests/content/browser/browser/base/content/test/browser_save_link-perwindowpb.js | finished in 30017ms
I have to touch this file, I might as well take it.
Assignee: nobody → mmc
The patch for bug 858234 might fix this bug.
Depends on: 858234
(In reply to Monica Chew [:mmc] from comment #23) > The patch for bug 858234 might fix this bug. Looks like that bug's a bit hung up?
Flags: needinfo?(mmc)
Paolo's been traveling to a workweek, I'll ping him. Thanks for the reminder!
Flags: needinfo?(mmc)
Unless you think it's ok to check in the part of the patch that ehsan already reviewed?
Comment on attachment 745438 [details] [diff] [review] Fix browser_save_link_perwindow_pb Review of attachment 745438 [details] [diff] [review]: ----------------------------------------------------------------- I'm looking into this patch since I already tested it as part of bug 858234 (and while here, I'd add a quick reminder to update the patch headers before landing this separately). Also, while working on bug 810490, I found this change to be necessary: function testOnWindow(options, callback) { var win = OpenBrowserWindow(options); win.addEventListener("load", function onLoad() { win.removeEventListener("load", onLoad, false); windowsToClose.push(win); - executeSoon(function() callback(win)); + win.BrowserChromeTest.runWhenReady(function () callback(win)); I suggest doing this while here, it may also help in reducing intermittent behavior. ::: browser/base/content/test/browser_save_link-perwindowpb.js @@ +9,5 @@ > let NetUtil = tempScope.NetUtil; > > // Trigger a save of a link in public mode, then trigger an identical save > // in private mode and ensure that the second request is differentiated from > +// the first by checking the cookies that are never sent. To clarify, "by checking that the cookies set by the first response are not sent during the second request", or something to that effect. @@ +18,1 @@ > testBrowser.loadURI("http://mochi.test:8888/browser/browser/base/content/test/bug792517-2.html"); "The server script linked by this page sets a cookie..." @@ +62,5 @@ > ok(downloadSuccess, "Link should have been downloaded successfully"); > aWindow.gBrowser.removeCurrentTab(); > > // Give the request a chance to finish > + executeSoon(function() aCallback(destDir)); I think executeSoon is unnecessary here, as the request has already finished and both "http-on-modify-request" and "http-on-examine-response" have been called for sure. Maybe the original comment referred to the on-disk file, but with the current version of the code we also know the file has been written completely at this point, so I'd just remove or rewrite the comment. @@ +129,5 @@ > testOnWindow(undefined, function(win) { > + // The first save from a regular window sets a cookie. > + triggerSave(win, function(destDir) { > + is(gNumSet, 1, "1 cookie should be set"); > + destDir.remove(true); At least when combined with bug 858234, this doesn't remove the temporary file created in the system temporary directory. If this works when this patch is applied separately, this issue may also be addressed in the other bug.
Attachment #745438 - Flags: feedback+
Thanks Paolo. > - executeSoon(function() callback(win)); > + win.BrowserChromeTest.runWhenReady(function () callback(win)); I don't understand what is going on here, but using runWhenReady gets a 404 on mozilla.org/projects/version/firstrun (which gets a 301 when I resolve it manually) > // Give the request a chance to finish > + executeSoon(function() aCallback(destDir)); > I think executeSoon is unnecessary here, as the request has already finished and > both "http-on-modify-request" and "http-on-examine-response" have been called > for sure. Unfortunately removing executeSoon produces a hang, for reasons I don't understand. Is it possible that the events have not yet arrived even though the request is finished? > + destDir.remove(true); > At least when combined with bug 858234, this doesn't remove the temporary file > created in the system temporary directory. I added ok(!destDir.exists()) and ok(!destFile.exists()) checks to make sure I unbreak this in the other patch.
Tree is closed. r=ehsan,paolo
Keywords: checkin-needed
This doesn't apply cleanly to m-c. Please rebase.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/244595f01a56
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Hmm, still flaking, but not as hard. Clearly the right thing to do is to use promises. I don't think I'll have time to do this anytime soon, though.
Closing since it hasn't flaked in 6 weeks.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The test is racy in that it sometimes manages to start loading the URI of the first window before delayedStartup(). But sometimes delayedStartup() cancels the load because it comes second and loads about:privatebrowsing. Waiting until browser-delayed-startup-finished has been sent does the trick. This is important to keep in mind when writing tests, never use a window for testing before the delayed startup finished.
Attachment #782168 - Flags: review?(ehsan)
Attachment #782168 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/1db0b845ce13
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Don't mind us, we'll just be piling on the "oh, and you're prone to timing out when we're OOM" failures until we finally stop with the OOMs.
Depends on: 937997
philor, none of the recent failures here are on Win7, so to me it looks unrelated to bug 937997, which seems to be us running out of virtual address space due to Windows graphics driver weirdness, rather than using too much memory and running out. Why did you mark it as depending on that bug? I'm also curious why these are OOM failures, as it doesn't look obvious to me.
Yeah, on second thought I probably should have marked it as depending on the incompletely-fixed first OOM closure bug. I'm not at all convinced that the switch to calling bug 937997 purely about Win7 is correct, but we'll see.
No longer depends on: 937997
On bc at least, the RSS doesn't go above 1.1gigs or so, which should be fine outside of fragmentation issues. Anyways, after the current crisis is over, I'm going to spend some of my time investigating Mochitest memory usage in bug 938691, so if you want to make any OOM bugs you come across block that bug or something it would give me something concrete to look at. Thanks.
reopen - its back https://tbpl.mozilla.org/php/getParsedLog.php?id=30691740&tree=Fx-Team
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I'm probably no longer the right person for this.
Assignee: mmc → nobody
Attached patch possible fix — — Splinter Review
I haven't reproduced this, nor has this showed up with remove-perf-mode, but since RyanVM asked whether there might be some fix for this... I think this is the same case as elsewhere. When loading the first page to a pb window, the about:privatebrowsing or some such may be loaded. So just try to load the page again.
Attachment #8350417 - Flags: review?(ehsan)
Attachment #8350417 - Flags: review?(ehsan) → review+
Keywords: checkin-needed
(just the last patch)
(In reply to TBPL Robot from comment #367) This is post-comment 366 :(
status-firefox27: --- → affected
status-firefox28: --- → affected
status-firefox29: --- → affected
status-firefox-esr24: --- → affected
Whiteboard: [leave open]
Target Milestone: Firefox 23 → ---
This has gotten a lot more failure-prone recently. We need a fix here or this test is heading for disabling.
Flags: needinfo?(gavin.sharp)
Summary: Intermittent browser_save_link-perwindowpb.js | Test timed out → Intermittent browser_save_link-perwindowpb.js | Test timed out | Found a browser window after previous test timed out
Blocks: fxdesktopbacklog
Flags: needinfo?(gavin.sharp)
Disabled on Linux. https://hg.mozilla.org/integration/fx-team/rev/1781bcd4deaa
Whiteboard: [leave open] → [test disabled on Linux][leave open]
Depends on: 982345
Assignee: nobody → ehsan
Whiteboard: [test disabled on Linux][leave open] → [test disabled on Linux][leave open] p=0
There is at least one thing that is wrong with this test, and that is, it doesn't ensure focus on the opened window before trying to synthesize a right click on the link in that page. I have a fix for that + a debug logging patch ready to go when/if the tree reopens!
Another thing that's quite broken is: aWindow.document.addEventListener("popupshown", function(e) contextMenuOpened(aWindow, e), false); // ... function contextMenuOpened(aWindow, event) { event.currentTarget.removeEventListener("popupshown", contextMenuOpened, false); // ... } This won't remove the event listener.
Also, "windowsToClose" is useless currently because we don't actually push any windows to it. Looks like the window is closed by the gBrowser.removeCurrentTab() call, though.
(In reply to Tim Taubert [:ttaubert] (away March 17th-21st) from comment #711) > Another thing that's quite broken is: > > aWindow.document.addEventListener("popupshown", function(e) > contextMenuOpened(aWindow, e), false); > > // ... > > function contextMenuOpened(aWindow, event) { > event.currentTarget.removeEventListener("popupshown", contextMenuOpened, > false); > > // ... > } > > This won't remove the event listener. Haha, indeed! But the good thing is that if we close the window properly then the damage is contained. (In reply to Tim Taubert [:ttaubert] (away March 17th-21st) from comment #712) > Also, "windowsToClose" is useless currently because we don't actually push > any windows to it. Looks like the window is closed by the > gBrowser.removeCurrentTab() call, though. I'll fix that too.
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #714) > I'll fix that too. Great, thanks! I looked into why running the test locally reports two leaked windows. One of them seems to be caused by the mockTransferCallback and we can fix it by doing: - mockTransferCallback = function(){}; + mockTransferCallback = null; I have no clue yet what causes the second remaining about:blank leak.
Another nit: NetUtil.jsm imported at the top is unused.
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #714) > (In reply to Tim Taubert [:ttaubert] (away March 17th-21st) from comment > #711) > > Another thing that's quite broken is: > > > > aWindow.document.addEventListener("popupshown", function(e) > > contextMenuOpened(aWindow, e), false); > > > > // ... > > > > function contextMenuOpened(aWindow, event) { > > event.currentTarget.removeEventListener("popupshown", contextMenuOpened, > > false); > > > > // ... > > } > > > > This won't remove the event listener. > > Haha, indeed! But the good thing is that if we close the window properly > then the damage is contained. > > (In reply to Tim Taubert [:ttaubert] (away March 17th-21st) from comment > #712) > > Also, "windowsToClose" is useless currently because we don't actually push > > any windows to it. Looks like the window is closed by the > > gBrowser.removeCurrentTab() call, though. > > I'll fix that too. Fixed these: https://hg.mozilla.org/integration/mozilla-inbound/rev/7035905efdc4
Even with a minimal test, I can reproduce the remaining leak just by loading a different URL in the initial tab. This doesn't seem related to the test and won't fix the intermittent issue anyway.
In this log <https://tbpl.mozilla.org/php/getParsedLog.php?id=36159557&full=1&branch=mozilla-inbound#error0>, I see: 12:09:35 INFO - TEST-START | chrome://mochitests/content/browser/browser/base/content/test/general/browser_save_link-perwindowpb.js 12:09:36 INFO - TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/general/browser_save_link-perwindowpb.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 be declared in the document or in the transfer protocol." {file: "http://mochi.test:8888/browser/browser/base/content/test/general/bug792517-2.html" line: 0}] 12:09:37 INFO - TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/general/browser_save_link-perwindowpb.js | Cookie should be foopy=1 12:09:37 INFO - TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/general/browser_save_link-perwindowpb.js | Link should have been downloaded successfully 12:09:37 INFO - TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/general/browser_save_link-perwindowpb.js | Destination dir should be removed 12:09:37 INFO - TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/general/browser_save_link-perwindowpb.js | Destination file should be removed 12:09:37 INFO - TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/general/browser_save_link-perwindowpb.js | 1 cookie should be set 12:09:38 INFO - TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/general/browser_save_link-perwindowpb.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 be declared in the document or in the transfer protocol." {file: "http://mochi.test:8888/browser/browser/base/content/test/general/bug792517-2.html" line: 0}] 12:09:38 INFO - JavaScript error: chrome://browser/content/browser.xul, line 1: gContextMenu is null 12:09:38 INFO - TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/general/browser_save_link-perwindowpb.js | Console message: [JavaScript Error: "TypeError: gContextMenu is null" {file: "chrome://browser/content/browser.xul" line: 1}] 12:10:06 INFO - Xlib: extension "RANDR" missing on display ":0". 12:10:07 WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_save_link-perwindowpb.js | Test timed out That message comes from one of the oncommand handlers (note the line number of 1). This is where gContextMenu is initialized: <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#310>. Does this mean that the popupshown event is dispatched after popuphiding?? That seems to be very broken.
I can reproduce that failure locally (although it takes a lot of time to run all those tests in my VM - and it only fails when running them all in test/general) and I see two failure modes: 1) This is the one you described above. "gContextMenu is null" errors are thrown. contextMenuOpened() got called and we're now waiting for MockFilePicker.showCallback() or mockTransferCallback() to be called. 2) No failure messages like in (1) but we hang and don't reach contextMenuOpened(), the pageshow listener is called. Maybe that's caused by a focus issue as you suspected already.
(In reply to Tim Taubert [:ttaubert] (away March 17th-21st) from comment #723) > I can reproduce that failure locally (although it takes a lot of time to run > all those tests in my VM - and it only fails when running them all in > test/general) and I see two failure modes: > > 1) This is the one you described above. "gContextMenu is null" errors are > thrown. contextMenuOpened() got called and we're now waiting for > MockFilePicker.showCallback() or mockTransferCallback() to be called. > > 2) No failure messages like in (1) but we hang and don't reach > contextMenuOpened(), the pageshow listener is called. Maybe that's caused by > a focus issue as you suspected already. Are you on latest inbound? If so, can you please attach your logs?
Flags: needinfo?(ttaubert)
So far no logs after my patches. Could this have just been the focus issue?
Maybe, yeah. I pulled latest inbound to try and give you some logs but I can't reproduce it anymore.
Flags: needinfo?(ttaubert)
I think we're winning here. Let's also re-enable browser_save_private_link_perwindowpb.js: https://hg.mozilla.org/integration/mozilla-inbound/rev/57ca7c65e7de And also perform the same kinds of cleanups on that test too: https://hg.mozilla.org/integration/mozilla-inbound/rev/0c3085556ba4 Leaving this open for some more time...
I hereby declare victory here.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 12 years ago
Resolution: --- → FIXED
status-b2g-v1.2: --- → affected
status-b2g-v1.3: --- → affected
status-b2g-v1.4: --- → fixed
status-firefox27: affected → wontfix
status-firefox28: affected → wontfix
status-firefox30: --- → fixed
Whiteboard: [test disabled on Linux][leave open] p=0 → p=0
Target Milestone: --- → Firefox 30
Whiteboard: p=0 → p=0 s=it-31c-30a-29b.2
Whiteboard: p=0 s=it-31c-30a-29b.2 → p=0 s=it-31c-30a-29b.2 [qa-]
Status: RESOLVED → VERIFIED
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: