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

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect, P2)

Description

[Ed Lee :Mardak]

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.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

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)

Comment 2

[(no longer active)]

(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.

Comment 4

[(no longer active)]

Attachment: Patch (v1)

This patch uses a shared js module to store a global in-memory path to the last save directory inside the private browsing mode.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 6

[(no longer active)]

(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.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(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.

Comment 8

[(no longer active)]

Mass moving of all Firefox::General private browsing bugs to Firefox::Private Browsing.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 10

[(no longer active)]

Attachment: Patch (v2)

Comments addressed.

Comment 11

[(no longer active)]

Attachment: Checked in [Checkin: Comment 11]

Comment 12

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/c8faab9608dc

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 14

[Mike Connor [:mconnor]]

Ehsan, any update here?

Comment 15

[(no longer active)]

(In reply to comment #14)
> Ehsan, any update here?

I will give this a look either later today or tomorrow.

Comment 16

[(no longer active)]

Attachment: Followup fix [Checkin: Comment 18]

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.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 18

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/53c8f565f839

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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?

Comment 20

[(no longer active)]

Yes, you're right.  I think there are some possibilities for creating an automated unit test for this, and I'll investigate this.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Marcia, I've disabled the existing test on Litmus for now:
https://litmus.mozilla.org/show_test.cgi?id=7442

Comment 23

[(no longer active)]

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.

Comment 24

[(no longer active)]

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?

Comment 25

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

I've filed bug 483481 for the Windows 7 issue.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 28

[Mike Connor [:mconnor]]

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.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Comment 30

[(no longer active)]

(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.

Comment 31

[(no longer active)]

Attachment: Tests

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.

Comment 32

[(no longer active)]

Since the patch I just posted tests all aspects of the code, no Litmus tests would be necessary.  Yay!  :-)

Comment 33

[Mike Beltzner [:beltzner, not reading bugmail]]

Ehsan, can we get a single rolled-up patch with the tests here?

Comment 34

[(no longer active)]

Attachment: Folded patch for 1.9.1

(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.

Comment 35

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 36

[(no longer active)]

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>

Comment 37

[(no longer active)]

Backed out the tests because of test failures on all three platforms.

I have not been able to reproduce the failures locally.  Still investigating.

Comment 38

[(no longer active)]

(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...

Comment 39

[(not currently active) Ted Mielczarek]

Serge did most of the work there, but I can take a look.

Comment 40

[(no longer active)]

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?

Comment 41

[Serge Gautherie (:sgautherie)]

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!

Comment 42

[Serge Gautherie (:sgautherie)]

(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.

Comment 43

[Serge Gautherie (:sgautherie)]

(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.

Comment 44

[(no longer active)]

(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.

Comment 45

[(no longer active)]

(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?

Comment 46

[Serge Gautherie (:sgautherie)]

(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

Comment 47

[(no longer active)]

(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.

Comment 48

[(not currently active) Ted Mielczarek]

Ehsan: are you using a debug build? I believe they re-register components more frequently.

Comment 49

[(no longer active)]

(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.

Comment 50

[(no longer active)]

(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...

Comment 51

[Serge Gautherie (:sgautherie)]

(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.

Comment 52

[(no longer active)]

Attachment: The new patch

Attached for reference

Comment 53

[Serge Gautherie (:sgautherie)]

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 54

[Serge Gautherie (:sgautherie)]

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.

Comment 55

[(no longer active)]

(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.

Comment 56

[(no longer active)]

Attachment: Tests (v2) [Checkin: Comment 57+59]

The new tests patch with Serge's comments addressed.

Comment 57

[(no longer active)]

The tests patch landed on trunk again as <http://hg.mozilla.org/mozilla-central/rev/de7218b522b0>.

Comment 58

[Mark Banner (:standard8)]

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.

Comment 59

[Mark Banner (:standard8)]

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.

Comment 60

[(no longer active)]

Ah, sorry Mark for not catching that in my patch, and thanks for the quick fix!

Comment 61

[(no longer active)]

Attachment: 1.9.1 Patch

Cumulative patch for 1.9.1 containing all the fixes for this bug and also bug 485187 (which is already approved).

Comment 62

[(no longer active)]

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.

Comment 63

[(no longer active)]

Attachment: 1.9.1 Patch [Checkin: Comment 69]

Fixed the test breakage, nominating for 1.9.1 approval again.

Comment 64

[:Gavin Sharp [email: [e-mail]]]

What caused the bustage, and how did you fix it?

Comment 65

[(no longer active)]

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.

Comment 66

[(no longer active)]

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>

Comment 67

[(no longer active)]

The try server unit tests on all the platforms with this patch were successful, except for the randomorange failures...

Comment 68

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 379636 [details] [diff] [review]
1.9.1 Patch
[Checkin: Comment 69]

a191=beltzner

Comment 69

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ac647e8ce075

Comment 70

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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