Closed Bug 464795 Opened 11 years ago Closed 11 years ago

Persist "save as" directory during private browsing, but restore previous value after

Categories

(Firefox :: Private Browsing, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: Mardak, Assigned: Ehsan)

References

Details

(Keywords: regression, verified1.9.1)

Attachments

(4 files, 6 obsolete files)

Bug 463888 made the "save as" directory not get saved at all during private browsing, but users who download multiple files to the same directory during private browsing will need to repick the directory each time.
Flags: blocking-firefox3.1?
Definitely nice to have, but not essential.

(I'm assuming that since we're preserving the original profile, the summary of this bug is slightly misleading in that even presently, when one exits Private Browsing mode, their preference of download location from non-private-browsing mode is restored)
Flags: wanted-firefox3.1+
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1-
(In reply to comment #1)
> (I'm assuming that since we're preserving the original profile, the summary of
> this bug is slightly misleading in that even presently, when one exits Private
> Browsing mode, their preference of download location from non-private-browsing
> mode is restored)

This bug is about persisting the "save as" directory *during* the private browsing mode, and restoring it to the value which was set last outside of the private mode after exiting this mode.  In fact, I think we want to store the last directory in memory during the private mode to make sure that the temporary pref never touches the disk.
Duplicate of this bug: 469230
Attached patch Patch (v1) (obsolete) — Splinter Review
This patch uses a shared js module to store a global in-memory path to the last save directory inside the private browsing mode.
Attachment #352774 - Flags: review?(gavin.sharp)
Why not hold on to the nsIFile itself rather than the path, to avoid having to re-create it? I think it would also be better to have the module explicitly clear its reference to the nsIFile when exiting private browsing mode (there's an observer topic for that, right?), rather than doing it in the code that uses it.
(In reply to comment #5)
> Why not hold on to the nsIFile itself rather than the path, to avoid having to
> re-create it?

Wouldn't it cause problems if the underlying folder gets deleted for example during the time we hold the nsIFile reference?

> I think it would also be better to have the module explicitly
> clear its reference to the nsIFile when exiting private browsing mode (there's
> an observer topic for that, right?), rather than doing it in the code that uses
> it.

I'll do this in the next revision of the patch.
(In reply to comment #6)
> Wouldn't it cause problems if the underlying folder gets deleted for example
> during the time we hold the nsIFile reference?

Ah, right... you use a getter that checks Exists() and releases the reference if it is false.
Mass moving of all Firefox::General private browsing bugs to Firefox::Private Browsing.
Component: General → Private Browsing
QA Contact: general → private.browsing
Comment on attachment 352774 [details] [diff] [review]
Patch (v1)

Canceling review request, since I'm assuming you're going to rework it per previous comments.
Attachment #352774 - Flags: review?(gavin.sharp)
Flags: in-litmus?
Attached patch Patch (v2) (obsolete) — Splinter Review
Comments addressed.
Attachment #352774 - Attachment is obsolete: true
Attachment #355561 - Flags: review?(gavin.sharp)
Attachment #355561 - Flags: review?(gavin.sharp) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/c8faab9608dc
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
Attachment #356668 - Attachment description: For check-in → Checked in
Attachment #356668 - Flags: approval1.9.1?
Sorry Ehsan, but that doesn't work with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090115 Minefield/3.2a1pre ID:20090115020316

Staying in PB mode and doing following steps still reverts to the initial download folder:

1. Enter PB mode
2. Open http://www.google.com
3. Save logo to another folder as the current one
4. Do step 3 again

In step 4 the initial folder of step 3 is selected again, instead of showing the target folder where the image was saved to.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 474152
Whiteboard: [PB-P2]
Ehsan, any update here?
Status: REOPENED → ASSIGNED
Priority: -- → P2
(In reply to comment #14)
> Ehsan, any update here?

I will give this a look either later today or tomorrow.
The problem in comment 13 is caused only in cases where browser.download.lastDir is not already set, which causes the code to get this pref to throw, which effectively prevents using gDownloadLastDir inside the private browsing mode, hence it would appear that the last dir is not saved correctly.

The attached follow-up fix changes the order of things so that the potentially throwing operation is only attempted outside of the private browsing mode where it's actually needed.
Attachment #366211 - Flags: review?(gavin.sharp)
Whiteboard: [PB-P2]
Comment on attachment 366211 [details] [diff] [review]
Followup fix
[Checkin: Comment 18]

I missed this in the original review, but having a variable whose name includes "path" (gDownloadLastDir.path/gDownloadLastDirPath) refer to an nsIFile instead of an actual file path is fairly confusing. Followup bug to fix?
Attachment #366211 - Flags: review?(gavin.sharp) → review+
http://hg.mozilla.org/mozilla-central/rev/53c8f565f839
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Attachment #366211 - Flags: approval1.9.1?
Looks better. Verified fixed with builds on OS X and Windows:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090311 Minefield/3.2a1pre ID:20090311030635

Shouldn't this better go into the testsuite as in Litmus?
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Yes, you're right.  I think there are some possibilities for creating an automated unit test for this, and I'll investigate this.
Whiteboard: [PB-P3]
Sorry, but I have to reopen this bug. This time it fails on Windows 7. A user on IRC reported that it is not working with 3.1b3 and I remembered this bug. So it is not fixed for 1.9.1 yet but I also made a test on Windows 7 and this bug is still there with Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090313 Minefield/3.2a1pre

Looks like something is different for that OS.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Marcia, I've disabled the existing test on Litmus for now:
https://litmus.mozilla.org/show_test.cgi?id=7442
Status: REOPENED → ASSIGNED
Hmm, strange.  I didn't realize that the code responsible is platform dependent.

Henrik, could you please report the exact steps to reproduce which you used on Windows 7?  If possible, please test this both with and without browser.download.lastDir pref being set.
Also, this patch touches two distinct places which can be invoked by saving an image, and saving a web page.  Can you test both cases?
This is getting ridiculous :)

Tracking the landing of more than one patch per bug is unmanageable. Henrik, in the future, can you avoid reopening bugs unless their patches have been backed out? Filing a new bug is almost always the right play.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
I've filed bug 483481 for the Windows 7 issue.
As long as a bug isn't fixed why a new bug should be filed? I know we already had this discussion a couple of times but see bug 475066 comment 21 too.
Because it grows increasingly difficult to understand the status of a bug when there's multiple reports and multiple issues being fixed.  If there's a specific case where something doesn't work (especially for beta OS versions!) filing a specific followup bug is much easier to track and understand.

Basic rule of thumb: if a bug isn't fixed at all by a checkin, we should back out the fix and reopen the bug.  If there is a specific case that wasn't addressed, or the fix doesn't work on a specific OS, it is best practice to file a new bug, so that we can address that specific case.  In this particular case, it works in all cases except apparently on Windows 7 (that bug needs more details, btw), so to say it's not fixed in the general case is more confusing than filing a new bug.  Now we have a bug that Gavin filed that is quite specific and easily understood by drivers, developers, random passerby, etc.  That's a much better situation overall.
Whiteboard: [PB-P3]
Ok, sounds reasonable. I'll run tests against Windows 7 and will report back in the new created bug.

Meanwhile this is verified on trunk with builds on OS X and Windows XP:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090322 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090322051216
Status: RESOLVED → VERIFIED
(In reply to comment #17)
> (From update of attachment 366211 [details] [diff] [review])
> I missed this in the original review, but having a variable whose name includes
> "path" (gDownloadLastDir.path/gDownloadLastDirPath) refer to an nsIFile instead
> of an actual file path is fairly confusing. Followup bug to fix?

Bug 485187 filed.  With a patch!  :-)  Sorry for the delay.
Attached patch Tests (obsolete) — Splinter Review
A full test suite for everything which was added in this bug.

I actually found a bug while writing these tests: gDownloadLastDir used to hold on to the nsIFile provided to it directly, and if that object was modified later by some other code, gDownloadLastDir would also end up with the modified object without ever knowing it.  This patch fixes this problem as well, and ensures that gDownloadLastDir doesn't plainly store a reference to the nsIFile provided to it in test_DownloadLastDir.js.
Attachment #372228 - Flags: review?(gavin.sharp)
Since the patch I just posted tests all aspects of the code, no Litmus tests would be necessary.  Yay!  :-)
Flags: in-litmus?
Blocks: 485187
Ehsan, can we get a single rolled-up patch with the tests here?
Attachment #356668 - Flags: approval1.9.1? → approval1.9.1-
Attachment #366211 - Flags: approval1.9.1? → approval1.9.1-
Attached patch Folded patch for 1.9.1 (obsolete) — Splinter Review
(In reply to comment #33)
> Ehsan, can we get a single rolled-up patch with the tests here?

Here is a folded patch which includes the patch from bug 485187 as well (which is already approved for 1.9.1).

Waiting on Gavin's review on the test patch before requesting approval on this one.
Attachment #372228 - Flags: review?(gavin.sharp) → review+
Comment on attachment 372228 [details] [diff] [review]
Tests

>diff --git a/toolkit/mozapps/downloads/src/DownloadLastDir.jsm b/toolkit/mozapps/downloads/src/DownloadLastDir.jsm

>   set file(val) {
>-    gDownloadLastDirFile = val;
>+    if (val instanceof Components.interfaces.nsIFile)
>+      gDownloadLastDirFile = val.clone();

Do we want to support clearing the saved value with .file = null? Doesn't appear to be required at the moment, but an "else gDownloadLastDirFile = null" probably wouldn't hurt.

Looks great otherwise, this will be a nice increase in this code's test coverage.
Landed with the requested changes: <http://hg.mozilla.org/mozilla-central/rev/c319b49e2880>

I also went ahead an added a test for the case where something other than an nsIFile is assigned to gDownloadLastDir.file:

<http://hg.mozilla.org/mozilla-central/rev/c319b49e2880#l3.58>
Flags: in-testsuite? → in-testsuite+
Backed out the tests because of test failures on all three platforms.

I have not been able to reproduce the failures locally.  Still investigating.
(In reply to comment #37)
> Backed out the tests because of test failures on all three platforms.
> 
> I have not been able to reproduce the failures locally.  Still investigating.

So, as it turns out, currently the xpcshell tests are being run twice, once using |make check| and once using |make xpcshell-tests|.  These failures happen in the latter run.  As can be seen, on the first run the tests pass successfully:

d:/mozilla-build/python25/python.exe -u \
          /e/builds/moz2_slave/mozilla-central-win32-unittest/build/testing/xpcshell/runxpcshelltests.py \
          ../../../../dist/bin/xpcshell \
          ../../../../_tests/xpcshell/test_downloads/unit
TEST-PASS | e:\builds\moz2_slave\mozilla-central-win32-unittest\build\objdir\_tests\xpcshell\test_downloads\unit\test_DownloadLastDir.js | all tests passed
TEST-PASS | e:\builds\moz2_slave\mozilla-central-win32-unittest\build\objdir\_tests\xpcshell\test_downloads\unit\test_DownloadUtils.js | all tests passed
TEST-PASS | e:\builds\moz2_slave\mozilla-central-win32-unittest\build\objdir\_tests\xpcshell\test_downloads\unit\test_privatebrowsing_downloadLastDir.js | all tests passed
TEST-PASS | e:\builds\moz2_slave\mozilla-central-win32-unittest\build\objdir\_tests\xpcshell\test_downloads\unit\test_syncedDownloadUtils.js | all tests passed
TEST-PASS | e:\builds\moz2_slave\mozilla-central-win32-unittest\build\objdir\_tests\xpcshell\test_downloads\unit\test_unspecified_arguments.js | all tests passed

And the second time with a failure:

TEST-PASS | e:\builds\moz2_slave\mozilla-central-win32-unittest\build\objdir\_tests\xpcshell\test_downloads\unit\test_DownloadLastDir.js | all tests passed
TEST-PASS | e:\builds\moz2_slave\mozilla-central-win32-unittest\build\objdir\_tests\xpcshell\test_downloads\unit\test_DownloadUtils.js | all tests passed
TEST-UNEXPECTED-FAIL | e:\builds\moz2_slave\mozilla-central-win32-unittest\build\objdir\_tests\xpcshell\test_downloads\unit\test_privatebrowsing_downloadLastDir.js | test failed, see following log:
  >>>>>>>
  ### XPCOM_MEM_LEAK_LOG defined -- logging leaks to c:\docume~1\cltbld\locals~1\temp\runxpcshelltests_leaks.log
*** Throwing trying to get CurProcD
*** test pending
*** Throwing trying to get CurWorkD
*** Throwing trying to get TmpD
*** Throwing trying to get DfltDwnld
*** Throwing trying to get Pers
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIFilePicker.init]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///e:/builds/moz2_slave/mozilla-central-win32-unittest/build/objdir/dist/bin/components/nsHelperAppDlg.js :: anonymous :: line 178"  data: no]
*** FAIL ***

  <<<<<<<
TEST-PASS | e:\builds\moz2_slave\mozilla-central-win32-unittest\build\objdir\_tests\xpcshell\test_downloads\unit\test_syncedDownloadUtils.js | all tests passed
TEST-PASS | e:\builds\moz2_slave\mozilla-central-win32-unittest\build\objdir\_tests\xpcshell\test_downloads\unit\test_unspecified_arguments.js | all tests passed


I'm not sure what's different between these two test running modes, but the problem seems to be that the do_load_module call at <http://hg.mozilla.org/mozilla-central/rev/c319b49e2880#l4.90> fails to correctly load my dummy nsIFilePicker implementation.

CCing Ted because IIRC he was the one who added the second round of xpcshell test runs...
Serge did most of the work there, but I can take a look.
Strange thing is everything passes for me locally by all of the following:

make check
make xpcshell-tests
make check ; make xpcshell-tests

Serge, do you have any suggestions here?
That may be a nit,
but having 2 tests named 'test_privatebrowsing_downloadLastDir.js' is not the easiest case to sort things out.
Could you rename them?

***

Thunderbird (all 3 platforms) reported (for |make check|):
{{
TEST-UNEXPECTED-FAIL | .../test_toolkit_general/unit/test_privatebrowsing_downloadLastDir.js | test failed, see following log:

*** test pending
TypeError: Cc['@mozilla.org/privatebrowsing;1'] is undefined
*** FAIL ***
}}

This needs fixing anyway!
(In reply to comment #38)
> using |make check| and once using |make xpcshell-tests|.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090417 Minefield/3.6a1pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/b427fd627d36)

I can reproduce.

> I'm not sure what's different between these two test running modes, but the

There should be none :-/

> problem seems to be that the do_load_module call at
> <http://hg.mozilla.org/mozilla-central/rev/c319b49e2880#l4.90> fails to
> correctly load my dummy nsIFilePicker implementation.

Let me see if I can find out more about this.
(In reply to comment #42)
> > I'm not sure what's different between these two test running modes, but the
> 
> There should be none :-/

And there is none, afaict: it just happens 'check' is run before 'xpcshell-tests'.

NB: Running the test alone is enough to reproduce, see below.

> > problem seems to be that the do_load_module call at
> > <http://hg.mozilla.org/mozilla-central/rev/c319b49e2880#l4.90> fails to
> > correctly load my dummy nsIFilePicker implementation.

I'm not sure what your 'correctly' means:
that line does not report any error ... whereas if I change the filename it does complain about it.

> Let me see if I can find out more about this.

What I'm seeing locally is that:
if 'objdir\dist\bin\components\compreg.dat' does not exist (= first run, or manually deleted), the test passes;
if it exists (= second run), I get comment 38 failure.


Back to you.
(In reply to comment #41)
> That may be a nit,
> but having 2 tests named 'test_privatebrowsing_downloadLastDir.js' is not the
> easiest case to sort things out.
> Could you rename them?
> 
> ***
> 
> Thunderbird (all 3 platforms) reported (for |make check|):
> {{
> TEST-UNEXPECTED-FAIL |
> .../test_toolkit_general/unit/test_privatebrowsing_downloadLastDir.js | test
> failed, see following log:
> 
> *** test pending
> TypeError: Cc['@mozilla.org/privatebrowsing;1'] is undefined
> *** FAIL ***
> }}
> 
> This needs fixing anyway!

I addressed both of these comments.
(In reply to comment #43)
> What I'm seeing locally is that:
> if 'objdir\dist\bin\components\compreg.dat' does not exist (= first run, or
> manually deleted), the test passes;
> if it exists (= second run), I get comment 38 failure.

OK, that's very weird.  I can't reproduce this with or without a compreg.dat in place from previous runs.  Can you please attach the compreg.dat file after running the first pass and the second pass?  Also could you please verify if compreg.dat is touched at the second time the unit test runs, and make sure it's not read-only somehow?
(In reply to comment #45)
> OK, that's very weird.  I can't reproduce this

Yes, weird that you can't reproduce.
Does it pass on Try servers?

> Can you please attach the compreg.dat file after
> running the first pass and the second pass?

I emailed you the files.

> Also could you please verify if
> compreg.dat is touched at the second time the unit test runs, and make sure
> it's not read-only somehow?

Not read-only.
compreg.dat is modified the first/successful time the test is run, not the second/failing one.

***

|make SOLO_FILE="test_privatebrowsing_downloadLastDir.js" -C objdir/toolkit/mozapps/downloads/tests check-one|

Failing line is
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in?mark=220#214
(In reply to comment #46)
> (In reply to comment #45)
> > OK, that's very weird.  I can't reproduce this
> 
> Yes, weird that you can't reproduce.

Yes.  I still can't... :-(

> Does it pass on Try servers?

Nope, the same failures.  I have played with the patch a bit and have submitted a new version to the try server, currently waiting on builds to finish there.

> > Can you please attach the compreg.dat file after
> > running the first pass and the second pass?
> 
> I emailed you the files.

Thanks.  I couldn't find anything suspicious there.

> > Also could you please verify if
> > compreg.dat is touched at the second time the unit test runs, and make sure
> > it's not read-only somehow?
> 
> Not read-only.
> compreg.dat is modified the first/successful time the test is run, not the
> second/failing one.

Same here, except that the second run succeeds as well.

> ***
> 
> |make SOLO_FILE="test_privatebrowsing_downloadLastDir.js" -C
> objdir/toolkit/mozapps/downloads/tests check-one|
> 
> Failing line is
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in?mark=220#214

In order to make sure that something fishy is not going on, I cloberred my objdir and pulled a clean version of m-c and applied my patch on top of it and built it.  Then I used exactly that command to run the test a few times successively, and it passed every time.

I'm not really sure how to proceed here.
Ehsan: are you using a debug build? I believe they re-register components more frequently.
(In reply to comment #48)
> Ehsan: are you using a debug build? I believe they re-register components more
> frequently.

I have tested both using a debug and a non-debug builds, and the test passes every time on both.
(In reply to comment #47)
> Nope, the same failures.  I have played with the patch a bit and have submitted
> a new version to the try server, currently waiting on builds to finish there.

The try server tests failed in a similar way...
(In reply to comment #44)
> I addressed both of these comments.

I suggest to _try_/attach/review/land the new version, but without the failing test, until the latter works.
Attached patch The new patch (obsolete) — Splinter Review
Attached for reference
Comment on attachment 374280 [details] [diff] [review]
The new patch

>+  try {
>+    pb = Cc["@mozilla.org/privatebrowsing;1"].
>+         getService(Ci.nsIPrivateBrowsingService);
>+  } catch (e) {
>+    // PB service is not available, bail out

(here and elsewhere) May as well "print" this comment so something (useful) shows up in the log.

>+    return;
>+  }

***

Did you drop privatebrowsing_downloadLastDir_filepicker.js on purpose?
Comment on attachment 374280 [details] [diff] [review]
The new patch

>+++ b/toolkit/content/tests/unit/test_downloadLastDir_privatebrowsing.js
>+++ b/toolkit/mozapps/downloads/tests/unit/test_privatebrowsing_downloadLastDir.js

Nit:
(Just swapping the names is good but could be confusing still,)
I'd suggest simply test_privatebrowsing_downloadLastDir_c.js and test_privatebrowsing_downloadLastDir_d.js.
(In reply to comment #52)
> Created an attachment (id=374280) [details]
> The new patch

This patch passed the tests successfully on the try server.  I'll address Serge's comments and post a new patch for review.  And the credit for suggesting a way to fix the problem goes to him as well!

(In reply to comment #53)
> (From update of attachment 374280 [details] [diff] [review])
> >+  try {
> >+    pb = Cc["@mozilla.org/privatebrowsing;1"].
> >+         getService(Ci.nsIPrivateBrowsingService);
> >+  } catch (e) {
> >+    // PB service is not available, bail out
> 
> (here and elsewhere) May as well "print" this comment so something (useful)
> shows up in the log.

Sure, will do.

> >+    return;
> >+  }
> 
> ***
> 
> Did you drop privatebrowsing_downloadLastDir_filepicker.js on purpose?

Yes, the implementation of the file picker component was moved to the test file itself.

(In reply to comment #54)
> (From update of attachment 374280 [details] [diff] [review])
> >+++ b/toolkit/content/tests/unit/test_downloadLastDir_privatebrowsing.js
> >+++ b/toolkit/mozapps/downloads/tests/unit/test_privatebrowsing_downloadLastDir.js
> 
> Nit:
> (Just swapping the names is good but could be confusing still,)
> I'd suggest simply test_privatebrowsing_downloadLastDir_c.js and
> test_privatebrowsing_downloadLastDir_d.js.

OK, no problem.  I'll do so in the next iteration of the patch.
The new tests patch with Serge's comments addressed.
Attachment #372228 - Attachment is obsolete: true
Attachment #374280 - Attachment is obsolete: true
Attachment #374500 - Flags: review?(gavin.sharp)
Depends on: 490273
Attachment #374500 - Flags: review?(gavin.sharp) → review?(johnath)
Attachment #374500 - Flags: review?(johnath) → review+
Keywords: checkin-needed
Whiteboard: [attachment #374500 needs to land on m-c]
The tests patch landed on trunk again as <http://hg.mozilla.org/mozilla-central/rev/de7218b522b0>.
Keywords: checkin-needed
Whiteboard: [attachment #374500 needs to land on m-c]
Somehow this managed to make the Thunderbird trunk unit tests fail:

TEST-UNEXPECTED-FAIL | /Volumes/Build/macosx-comm-central-check/build/objdir/mozilla/_tests/xpcshell/test_downloads/unit/test_DownloadLastDir.js | test failed (with xpcshell return code: 0), see following log:
  >>>>>>>
  ### XPCOM_MEM_LEAK_LOG defined -- logging leaks to /tmp/runxpcshelltests_leaks.log
*** test pending
TypeError: Cc['@mozilla.org/privatebrowsing;1'] is undefined
*** FAIL ***

  <<<<<<<

Although the TypeError is dumped out, I'm not sure that's the issue. I'm currently doing a build to see if I can work it out.
I've pushed http://hg.mozilla.org/mozilla-central/rev/429e151d6fdb to fix the Thunderbird trunk unit tests bustage.

Once I investigated it was clear that it just needed a copy and paste of the private browsing check that had made it into the other two tests in the original patch but hadn't made it into test_DownloadLastDir.js.
Ah, sorry Mark for not catching that in my patch, and thanks for the quick fix!
Attached patch 1.9.1 Patch (obsolete) — Splinter Review
Cumulative patch for 1.9.1 containing all the fixes for this bug and also bug 485187 (which is already approved).
Attachment #379552 - Flags: approval1.9.1?
Attachment #372957 - Attachment is obsolete: true
Comment on attachment 379552 [details] [diff] [review]
1.9.1 Patch

The linux unit test box failed:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1243254028.1243259864.24900.gz

EST-UNEXPECTED-FAIL | /builds/slave/sendchange-linux-unittest/mozilla/objdir/_tests/xpcshell/test_downloads/unit/test_privatebrowsing_downloadLastDir.js | test failed (with xpcshell return code: 0), see following log:
  >>>>>>>
  ### XPCOM_MEM_LEAK_LOG defined -- logging leaks to /tmp/runxpcshelltests_leaks.log
*** Throwing trying to get CurProcD
*** test pending
*** Throwing trying to get TmpD
*** Throwing trying to get DfltDwnld
*** Throwing trying to get Home
[Exception... "'[JavaScript Error: "aLeafName is null" {file: "file:///builds/slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsHelperAppDlg.js" line: 249}]' when calling method: [nsIHelperAppLauncherDialog::promptForSaveToFile]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: /builds/slave/sendchange-linux-unittest/mozilla/objdir/_tests/xpcshell/test_downloads/unit/test_privatebrowsing_downloadLastDir.js :: run_test :: line 218"  data: yes]
*** FAIL ***

I'll investigate this shortly.
Attachment #379552 - Flags: approval1.9.1?
Fixed the test breakage, nominating for 1.9.1 approval again.
Attachment #379552 - Attachment is obsolete: true
Attachment #379636 - Flags: approval1.9.1?
What caused the bustage, and how did you fix it?
validateLeafName in nsHelperAppDlg implementation on 1.9.1 did not handle a null leaf name parameter correctly, and promptForSaveFile was passing null to it (which came from the aDefaultFile parameter that I was passing to it).

Since we don't want promptForSaveFile in this test to use the default downloads directory anyway, I passed true as the "force prompt" parameter, which effectively bypasses the entire default download directory branch inside of promptForSaveFile, hence avoiding this bustage and giving us what we want.
To elaborate more, isUsableDirectory in this line <http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in#322> in m-c fails and therefore validateLeafName returns null and promptForSaveToFile skips the branch here: <http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in#210>
The try server unit tests on all the platforms with this patch were successful, except for the randomorange failures...
Comment on attachment 379636 [details] [diff] [review]
1.9.1 Patch
[Checkin: Comment 69]

a191=beltzner
Attachment #379636 - Flags: approval1.9.1? → approval1.9.1+
Depends on: 489077
Attachment #379636 - Attachment description: 1.9.1 Patch → 1.9.1 Patch [Checkin: Comment 69]
Attachment #374500 - Attachment description: Tests (v2) → Tests (v2) [Checkin: Comment 57]
Attachment #366211 - Attachment description: Followup fix → Followup fix [Checkin: Comment 18]
Attachment #356668 - Attachment description: Checked in → Checked in [Checkin: Comment 11]
Attachment #355561 - Attachment is obsolete: true
Blocks: 474152
No longer depends on: 474152
No longer depends on: 489077
Attachment #374500 - Attachment description: Tests (v2) [Checkin: Comment 57] → Tests (v2) [Checkin: Comment 57+59]
Verified fixed on 1.9.1 with builds on all platforms: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090527 Shiretoko/3.5pre ID:20090527031214
Depends on: 508526
Duplicate of this bug: 583663
You need to log in before you can comment on or make changes to this bug.