Closed Bug 857427 Opened 7 years ago Closed 6 years ago

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


(Firefox :: Private Browsing, defect)

Not set



Firefox 30
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


(Reporter: philor, Assigned: ehsan)



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


(3 files, 1 obsolete file)
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
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);
-      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 (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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
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.
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
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+
Closed: 6 years ago6 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
Resolution: FIXED → ---
I'm probably no longer the right person for this.
Assignee: mmc → nobody
Duplicate of this bug: 942518