DownloadLastDir.jsm uses global Private Browsing state to make decisions

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

unspecified
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=jdm][lang=js], )

Attachments

(7 attachments, 38 obsolete attachments)

11.73 KB, patch
Details | Diff | Splinter Review
12.15 KB, patch
Details | Diff | Splinter Review
7.76 KB, patch
Details | Diff | Splinter Review
21.91 KB, patch
Details | Diff | Splinter Review
5.32 KB, patch
Details | Diff | Splinter Review
8.73 KB, patch
Details | Diff | Splinter Review
6.31 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
The global state is going away. The interface only takes a URI right now; we should probably make it take a docshell and use that to query the PB state.
I think we can use the exact same strategy as bug 722988 for this one. Convert the gDownloadLastDir singleton to a instance that takes a window parameter, and replace the global check with a check of the stored window.
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [mentor=jdm][lang=js]
Posted patch WIP(Patch v1) (obsolete) — Splinter Review
Assignee: nobody → saurabhanandiit
Status: NEW → ASSIGNED
Attachment #633664 - Flags: feedback?(josh)
Attachment #633664 - Flags: feedback?(ehsan)
Comment on attachment 633664 [details] [diff] [review]
WIP(Patch v1)

Review of attachment 633664 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, I can review a version of this patch which addresses the below comments.

Have you run the patch through the try server yet?

::: toolkit/mozapps/downloads/DownloadLastDir.jsm
@@ +93,5 @@
> +  this._window = aWindow;
> +}
> +
> +DownloadLastDir.prototype = {
> +  isPrivate: function DownloadLastDir_isPrivate(aWindow) {

You don't need to pass this._window as an argument here since you can already access it from within the function.

@@ +103,5 @@
> +                         .getInterface(Ci.nsIDOMWindow)
> +                         .wrappedJSObject;
> +    
> +    if (temp != null)
> +      return temp.gPrivateBrowsingUI.privateWindow;

You should return fall if this condition doesn't hold.
Attachment #633664 - Flags: feedback?(josh)
Attachment #633664 - Flags: feedback?(ehsan)
Attachment #633664 - Flags: feedback+
This patch doesn't appear to update any code that imports the jsm.
(In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment #5)
> This patch doesn't appear to update any code that imports the jsm.

Oh good point, I missed that.
Posted patch WIP(Patch v2) (obsolete) — Splinter Review
I'm currently getting an error : 

TEST-UNEXPECTED-FAIL | xpcshell/head.js | [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: /home/saurabh/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/toolkit/mozapps/downloads/tests/unit/test_privatebrowsing_downloadLastDir.js :: run_test :: line 59"  data: no]

upon running the xpcshell test on the file |test_privatebrowsing_downloadLastDir.js| . I'm looking into the causes. I just wanted to confirm if I'm heading in the right direction.
Attachment #633664 - Attachment is obsolete: true
Attachment #636153 - Flags: feedback?(josh)
The error is basically coming in this call :

let launcherDialog = Cc["@mozilla.org/helperapplauncherdialog;1"].
                       getService(Ci.nsIHelperAppLauncherDialog);

in test_privatebrowsing_downloadLastDir.js. I suspect that the changes might require some new uuid. Any ideas ?
(In reply to Saurabh Anand [:sawrubh] from comment #8)
> The error is basically coming in this call :
> 
> let launcherDialog = Cc["@mozilla.org/helperapplauncherdialog;1"].
>                        getService(Ci.nsIHelperAppLauncherDialog);
> 
> in test_privatebrowsing_downloadLastDir.js. I suspect that the changes might
> require some new uuid. Any ideas ?

No, this error doesn't make any sense!  Are you should you had a full build?
The problem is that there is no window in nsHelperAppDlg.js. I tried using parent <http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js#244> in the constructor for gDownloadLastDir but it isn't working. It's failing at the QI : 

.getInterface(Ci.nsIWebNavigation)

The signature of parent and aContext are http://pastebin.mozilla.org/1678392 . Neither of them are chrome i.e. have gPrivateBrowsingUI in their signatures. I'm not sure which QI goop to use or whether to add some interfaces which can be queried to <http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/unit/test_privatebrowsing_downloadLastDir.js#17> .

Any ideas ?
The context object constructed in the test is just a placeholder object, which is obviously not a valid window object.  You need to convert this test into a browser-chrome test, so that you can have a real window object that you can use as the context.
Should I only change one of the xpcshell tests which is failing to mochitest-browser-chrome or all of the xpcshell test to their corresponding mochitest ?

@Josh, you said that the toolkit code doesn't have a window and we would have to work with nsILoadCotext. Does that apply here too ? If that is the case then I'll have to rework the patch from scratch.
(In reply to Saurabh Anand [:sawrubh] from comment #12)
> Should I only change one of the xpcshell tests which is failing to
> mochitest-browser-chrome or all of the xpcshell test to their corresponding
> mochitest ?

For the purpose of this bug, you can just change the failing test.  But please file a follow-up for changing the rest as well.
Blocks: 723002
Comment on attachment 636153 [details] [diff] [review]
WIP(Patch v2)

Yes, this patch will also need to obtain a load context instead of using gPrivateBrowsingUI. Sorry for not noticing it sooner. Furthermore, the private-browsing observer should be changed to last-pb-context-exited, and the pbSvc-related stuff should be removed.
Attachment #636153 - Flags: feedback?(josh)
The use of window.opener in nsHelperAppDlg.js is broken, as that is a component that is instantiated and does not have access to a window variable. There's a quick way to fix this: move the gDownloadLastDir initialization into the promptForSaveToFile method and use parent instead of window.opener.
This is a very crude conversion of the concerned xpcshell test to a mochitest.  tried to make it async, there might be(very likely) faults with this test.

Points to Note :
a) There is a FIXME, in this patch. @Josh, @Ehsan can you guys answer the solution to it please ?
b) I have removed the do_get_profile() call in the xpcshell test with FileUtils.getDir(), since I think it does the same thing. Please see if this is correct.
Attachment #638289 - Flags: feedback?(josh)
Attachment #638289 - Flags: feedback?(ehsan)
Fixed some nits, I noticed.

The previous comments still holds. Please clarify them. Thanks.
Attachment #638289 - Attachment is obsolete: true
Attachment #638289 - Flags: feedback?(josh)
Attachment #638289 - Flags: feedback?(ehsan)
Attachment #638303 - Flags: feedback?(josh)
Attachment #638303 - Flags: feedback?(ehsan)
Part 2 - This converts <dxr.lanedo.com/mozilla-central/toolkit/content/tests/unit/test_privatebrowsing_downloadLastDir_c.js.html> to a mochitest.

Points to Note :
a) There are some FIXME in this, please answer the questions/suggestions asked.
Attachment #638309 - Flags: feedback?(josh)
Attachment #638309 - Flags: feedback?(ehsan)
Attachment #638303 - Attachment description: Conversion of test_privatebrowsing_downloadLastDir.js to mochitest(v1) → Part 1 - Conversion of test_privatebrowsing_downloadLastDir.js to mochitest(v1)
Attachment #636153 - Attachment is obsolete: true
Sorry the url pasted in the description of the attachment of part 2 is <http://dxr.lanedo.com/mozilla-central/toolkit/content/tests/unit/test_privatebrowsing_downloadLastDir_c.js.html> , I forgot to add the "http://" :).
I am converting <http://dxr.lanedo.com/mozilla-central/toolkit/mozapps/downloads/tests/unit/test_DownloadLastDir.js.html> to mochitest. I had a question, do all mochitests have to be async or is there is some tips to keep in mind to decide whether to make it async or not ?
The do_get_profile() also registers a cleanup callback <http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/head.js#730>, Now that I no longer include this function call hence that cleanup is also not registered. @Josh, @Ehsan do you think that the cleanup done, in the xpcshell tests I've changed, besides this |do_register_cleanup| is enough or do I also have to register the cleanup functions in |do_register_cleanup| ?
Points to Note (Well, the entire patch for start ;) :
a) This changes <http://dxr.lanedo.com/mozilla-central/toolkit/mozapps/downloads/tests/unit/test_DownloadLastDir.js.html> to mochitest.
Attachment #638359 - Flags: feedback?(josh)
Attachment #638359 - Flags: feedback?(ehsan)
Points to Note :
a) This converts this <http://dxr.lanedo.com/mozilla-central/toolkit/mozapps/downloads/tests/unit/test_DownloadLastDirWithCPS.js.html> to mochitest.
b) There are a couple of FIXME's in the code, which need to be solved.
c) Check whether we need the Asynchronous implementation.
d) The async implementation done is right or not.
Attachment #638450 - Flags: feedback?(josh)
Attachment #638450 - Flags: feedback?(ehsan)
Finally all the test conversions are over. Now I'm going to upload the actual changes to the JSM's taking into consideration, Josh's and Ehsan's comments.
Points to Note :

a) In all the tests I've passed |window| and not |window.opener| to the DownloadLastDir constructors. Please check if this is what should be done or the other way round.
b) Moved the gDownloadLastDir declaration in the nsHelperAppDlg.js to the |promptForSaveToFile| and passed |parent| to the constructor.
c) Using nsILoadContext to obtain the PB flag. Since Bug 769467 hasn't yet landed, I'll continue QI's myself and later file a bug for refactoring this code to use the newly defined PB module.
Attachment #638481 - Flags: feedback?(josh)
Attachment #638481 - Flags: feedback?(ehsan)
Attachment #638481 - Attachment description: Part 5 - Actual Changes to the JSM's → Part 5 - Actual Changes to the JSM's(v1)
Comment on attachment 638303 [details] [diff] [review]
Part 1 - Conversion of test_privatebrowsing_downloadLastDir.js to mochitest(v1)

Review of attachment 638303 [details] [diff] [review]:
-----------------------------------------------------------------

This looks mostly good.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadLastDir.js
@@ +21,5 @@
> +
> +function test() {
> +  let prefsService = Cc["@mozilla.org/preferences-service;1"].
> +                     getService(Ci.nsIPrefService).
> +                     QueryInterface(Ci.nsIPrefBranch);

You can use gPrefService.

@@ +26,5 @@
> +  prefsService.setBoolPref("browser.privatebrowsing.keep_current_session", true);
> +  let prefs = prefsService.getBranch("browser.download.");
> +  let launcherDialog = Cc["@mozilla.org/helperapplauncherdialog;1"].
> +                       getService(Ci.nsIHelperAppLauncherDialog);
> +  let tmpDir = FileUtils.getDir("TmpD");

This is fine.

@@ +29,5 @@
> +                       getService(Ci.nsIHelperAppLauncherDialog);
> +  let tmpDir = FileUtils.getDir("TmpD");
> +  function newDirectory() {
> +    let dir = tmpDir.clone();
> +    dir.append("testdir" + Math.floor(Math.random() * 10000));

You don't need to append a random string here.  createUnique takes care of this for you.

@@ +35,5 @@
> +    return dir;
> +  }
> +  function newFileInDirectory(dir) {
> +    let file = dir.clone();
> +    file.append("testfile" + Math.floor(Math.random() * 10000));

Ditto.

@@ +53,5 @@
> +
> +  waitForExplicitFinish();
> +  
> +  downloadTest(function () {
> +    ok(!!file, "");

What's |file|?

@@ +67,5 @@
> +
> +      downloadTest(function () {
> +          // FIXME Not Sure whether setting displayDirectory equal to null, after
> +          // having called promptForSaveToFile in downloadTest will work or not.
> +          MockFilePicker.displayDirectory = null;

It sets mDisplayDirectory <http://dxr.lanedo.com/mozilla-central/widget/xpwidgets/nsBaseFilePicker.cpp.html#l162>.  Not sure what you expect this to do...
Attachment #638303 - Flags: feedback?(ehsan) → feedback+
> > +function test() {
> > +  let prefsService = Cc["@mozilla.org/preferences-service;1"].
> > +                     getService(Ci.nsIPrefService).
> > +                     QueryInterface(Ci.nsIPrefBranch);
> 
> You can use gPrefService.

Please use Services.prefs.
Comment on attachment 638309 [details] [diff] [review]
Part 2 - Conversion of test_privatebrowsing_downloadLastDir_c.js to mochitest(v1)

Review of attachment 638309 [details] [diff] [review]:
-----------------------------------------------------------------

My comments from the other test apply here as well.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadLastDir_c.js
@@ +8,5 @@
> +               getService(Ci.mozIJSSubScriptLoader);
> +  loader.loadSubScript("chrome://global/content/contentAreaUtils.js");
> +  Cu.import("resource://gre/modules/DownloadLastDir.jsm");
> +  Cu.import("resource://gre/modules/FileUtils.jsm");
> +}

I'd just do this stuff at the top level.

@@ +17,5 @@
> +function test() {
> +  loadUtilsScript();
> +  let prefsService = Cc["@mozilla.org/preferences-service;1"].
> +                     getService(Ci.nsIPrefService).
> +                     QueryInterface(Ci.nsIPrefBranch);

Use gPrefService.

@@ +50,5 @@
> +    displayDirectory: null,
> +    file: file1
> +  };
> +  // FIXME Why is this makeFilePicker being inserted into the global scope ?
> +  makeFilePicker = function() fp;

This code <http://dxr.lanedo.com/mozilla-central/toolkit/content/contentAreaUtils.js.html#l574> calls this function.  By overriding it, we make sure that a real file picker doesn't get opened.  I think this was done before the mock file picker helper existed.  You should just use the mock file picker, and should not need to overwrite makeFilePicker this way.

@@ +84,5 @@
> +
> +  waitForExplicitFinish();
> +  
> +  // FIXME I am not able to abstract much of the code to a separate function, since it involves
> +  // a call to |fp.file|, which will involve inserting |fp| to a global scope. Any workarounds ?

It's fine, I often write tests with nested anonymous functions like this myself!

@@ +113,5 @@
> +      
> +      // cleanup
> +      Cc["@mozilla.org/observer-service;1"]
> +        .getService(Ci.nsIObserverService)
> +        .notifyObservers(null, "quit-application", null);

Nit: use Services.obs.
Attachment #638309 - Flags: feedback?(ehsan) → feedback+
(In reply to Saurabh Anand [:sawrubh] from comment #20)
> I am converting
> <http://dxr.lanedo.com/mozilla-central/toolkit/mozapps/downloads/tests/unit/
> test_DownloadLastDir.js.html> to mochitest. I had a question, do all
> mochitests have to be async or is there is some tips to keep in mind to
> decide whether to make it async or not ?

There are some operations which happen asynchronously.  Switching in and out of the PB mode is one of them, but in general your test should only be aware of this if it initiates asynchronous stuff.

(In reply to Saurabh Anand [:sawrubh] from comment #21)
> The do_get_profile() also registers a cleanup callback
> <http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/head.js#730>,
> Now that I no longer include this function call hence that cleanup is also
> not registered. @Josh, @Ehsan do you think that the cleanup done, in the
> xpcshell tests I've changed, besides this |do_register_cleanup| is enough or
> do I also have to register the cleanup functions in |do_register_cleanup| ?

do_get_profile is only needed in xpcshell tests where we don't have a profile directory by default.  When running tests in any of the mochitest frameworks, you already have access to a profile directory, so calling do_get_profile or any of the things it does is not needed.
(In reply to Dão Gottwald [:dao] from comment #27)
> > > +function test() {
> > > +  let prefsService = Cc["@mozilla.org/preferences-service;1"].
> > > +                     getService(Ci.nsIPrefService).
> > > +                     QueryInterface(Ci.nsIPrefBranch);
> > 
> > You can use gPrefService.
> 
> Please use Services.prefs.

Heh, right.  :-)
Comment on attachment 638303 [details] [diff] [review]
Part 1 - Conversion of test_privatebrowsing_downloadLastDir.js to mochitest(v1)

Review of attachment 638303 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadLastDir.js
@@ +48,5 @@
> +  let file2 = newFileInDirectory(dir2);
> +  let file3 = newFileInDirectory(dir3);
> +  let context = gBrowser.selectedBrowser;
> +
> +  prefs.setComplexValue("lastDir", Ci.nsILocalFile, tmpDir);

Ci.nsILocalFile isn't actually a thing anymore. Use Ci.nsIFile instead.

@@ +59,5 @@
> +    is(MockFilePicker.displayDirectory.path, tmpDir.path, "");
> +    // browser.download.lastDir should be modified before entering the private browsing mode
> +    is(prefs.getComplexValue("lastDir", Ci.nsILocalFile).path, dir1.path, "");
> +    // gDownloadLastDir should be usable outside of the private browsing mode
> +    is(gDownloadLastDir.file.path, dir1.path, "");

This does not exist yet, does it? You just import the JSM above but don't create the gDownloadLastDir object like the original patch.

@@ +90,5 @@
> +              is(prefs.getComplexValue("lastDir", Ci.nsILocalFile).path, dir3.path, "");
> +              // gDownloadLastDir should be usable after leaving the private browsing mode
> +              is(gDownloadLastDir.file.path, dir3.path, "");
> +              
> +              // cleanup

Use registerCleanupFunction for all of this, please.
Attachment #638303 - Flags: feedback?(josh)
Comment on attachment 638359 [details] [diff] [review]
Part 3 - Conversion of test_DownloadLastDir.js to mochitest(v1)

Review of attachment 638359 [details] [diff] [review]:
-----------------------------------------------------------------

+ comments from the other tests.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDir.js
@@ +6,5 @@
> +let tmp = {};
> +Cu.import("resource://gre/modules/DownloadLastDir.jsm", tmp);
> +Cu.import("resource://gre/modules/FileUtils.jsm");
> +
> +let gDownloadLastDir = new tmp.DownloadLastDir(window.opener);

Don't use opener here.  These tests are loaded in an existing browser window, so you can use window (which points to the browser window) directly.

@@ +15,5 @@
> +  function clearHistory() {
> +    // simulate clearing the private data
> +    Cc["@mozilla.org/observer-service;1"].
> +    getService(Ci.nsIObserverService).
> +    notifyObservers(null, "browser:purge-session-history", "");

Nit: Services.obs.

@@ +18,5 @@
> +    getService(Ci.nsIObserverService).
> +    notifyObservers(null, "browser:purge-session-history", "");
> +  }
> +
> +  is(typeof gDownloadLastDir, "object", "");

Nit: here and in other places, please pass a useful diagnostic message to the test functions.  For example, "Make sure gDownloadLastDir is a valid object".
Attachment #638359 - Flags: feedback?(ehsan) → feedback+
Comment on attachment 638450 [details] [diff] [review]
Part 4 - Coversion of test_DownloadLastDirWithCPS.js to mochitest(v1)

Review of attachment 638450 [details] [diff] [review]:
-----------------------------------------------------------------

+ the comments from the other tests.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js
@@ +6,5 @@
> +let tmp = {};
> +Cu.import("resource://gre/modules/DownloadLastDir.jsm", tmp);
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/FileUtils.jsm");
> +let gDownloadLastDir = new tmp.DownloadLastDir(window.opener);

Same as the other case.

@@ +197,5 @@
> +                                                            setFileCallBack(function () {
> +                                                              is(gDownloadLastDir.getFile(uri3).path, dir3.path, "");
> +                                                              
> +                                                              setFileCallBack(function () {
> +                                                                is(gDownloadLastDir.getFile(uri3), null, "");

The nesting level here is too deep!  Can you please split this into multiple chunks, each in its own function?

@@ +253,5 @@
> +}
> +
> +function setFileCallBack(aCallback, firstURI, secondURI) {
> +  gDownloadLastDir.setFile(firstURI, secondURI);
> +  // FIXME Check whether gBrowser is available in this test or not.

It is.
Attachment #638450 - Flags: feedback?(ehsan) → feedback+
Comment on attachment 638309 [details] [diff] [review]
Part 2 - Conversion of test_privatebrowsing_downloadLastDir_c.js to mochitest(v1)

Review of attachment 638309 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadLastDir_c.js
@@ +63,5 @@
> +  validateFileName = function(foo) foo;
> +
> +  let params = {
> +    fpTitleKey: "test",
> +    // FIXME Is this FileInfo() available to me in this environment ?

Yes. http://mxr.mozilla.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#453

@@ +77,5 @@
> +  ok(getTargetFile(params), "");
> +  // file picker should start with browser.download.lastDir
> +  is(fp.displayDirectory.path, tmpDir.path, "");
> +  // browser.download.lastDir should be modified before entering the private browsing mode
> +  is(prefs.getComplexValue("lastDir", Ci.nsILocalFile).path, dir1.path, "");

s/nsILoadFile/nsIFile/

@@ +110,5 @@
> +      is(prefs.getComplexValue("lastDir", Ci.nsILocalFile).path, dir3.path, "");
> +      // gDownloadLastDir should be usable after leaving the private browsing mode
> +      is(gDownloadLastDir.file.path, dir3.path, "");
> +      
> +      // cleanup

registerCleanupFunction
Attachment #638309 - Flags: feedback?(josh)
Comment on attachment 638481 [details] [diff] [review]
Part 5 - Actual Changes to the JSM's(v1)

Review of attachment 638481 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDir.js
@@ +7,4 @@
>  Cu.import("resource://gre/modules/DownloadLastDir.jsm", tmp);
>  Cu.import("resource://gre/modules/FileUtils.jsm");
>  
> +let gDownloadLastDir = new tmp.DownloadLastDir(window);

As I said before, you should use |window| in the first place, no need to change things in the test here.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadLastDir.js
@@ +3,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +let tmp = {};

Nit: here and elsewhere, please use a better name!  :-)

::: toolkit/mozapps/downloads/DownloadLastDir.jsm
@@ +95,5 @@
> +    return this.window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                      .getInterface(Ci.nsIWebNavigation)
> +                      .QueryInterface(Ci.nsILoadContext)
> +                      .usePrivateBrowsing;
> +  },

Please use the API which landed in bug 769467.
Attachment #638481 - Flags: feedback?(ehsan) → feedback+
Please make sure that your tests patches finally pass the unit tests without the code changes, just to help ensure that the tests were valid both before and after the code changes.
Comment on attachment 638359 [details] [diff] [review]
Part 3 - Conversion of test_DownloadLastDir.js to mochitest(v1)

Review of attachment 638359 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDir.js
@@ +24,5 @@
> +  let tmpDir = FileUtils.getDir("TmpD");
> +  let newDir = tmpDir.clone();
> +
> +  newDir.append("testdir" + Math.floor(Math.random() * 10000));
> +  newDir.QueryInterface(Ci.nsILocalFile);

This line is unnecessary.

@@ +57,5 @@
> +            clearHistory();
> +            is(gDownloadLastDir.file, null, "");
> +            togglePrivateBrowsing(function () {
> +              is(gDownloadLastDir.file, null, "");
> +              prefs.clearUserPref("browser.privatebrowsing.keep_current_session");

registerCleanupFunction
Attachment #638359 - Flags: feedback?(josh)
Comment on attachment 638450 [details] [diff] [review]
Part 4 - Coversion of test_DownloadLastDirWithCPS.js to mochitest(v1)

Review of attachment 638450 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js
@@ +231,5 @@
> +          }, uri3, dir3);
> +        }, uri2, dir2);
> +      }, uri1, dir1);
> +    }, null, tmpDir);
> +  } finally {

Use registerCleanupFunction and avoid one level of indentation :)

@@ +240,5 @@
> +
> +  }
> +}
> +
> +function togglePrivateBrowsing(aCallback) {

This method shouldn't be necessary, as explained on IRC.

@@ +251,5 @@
> +
> +  pb.privateBrowsingEnabled = !pb.privateBrowsingEnabled;
> +}
> +
> +function setFileCallBack(aCallback, firstURI, secondURI) {

This doesn't need to be asynchronous. You can just inline all uses of setFileCallback with the gDownloadLastDir.setFile call.
Attachment #638450 - Flags: feedback?(josh)
Comment on attachment 638481 [details] [diff] [review]
Part 5 - Actual Changes to the JSM's(v1)

Review of attachment 638481 [details] [diff] [review]:
-----------------------------------------------------------------

Ehsan's feedback mirrors mine. This is all very close; how exciting!
Attachment #638481 - Flags: feedback?(josh)
Thank you Josh and Ehsan for the detailed feedbacks. I will get down to addressing your comments right aways.
Addressed the comments of Ehsan and Josh, regarding this patch.
Attachment #638481 - Attachment is obsolete: true
Attachment #640205 - Flags: feedback?(josh)
Attachment #640205 - Flags: feedback?(ehsan)
Comment on attachment 640205 [details] [diff] [review]
Part 5 - Actual Changes to the JSM's(v2)

Review of attachment 640205 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/downloads/DownloadLastDir.jsm
@@ +28,2 @@
>  const SAVE_PER_SITE_PREF = LAST_DIR_PREF + ".savePerSite";
>  const PBSVC_CID = "@mozilla.org/privatebrowsing;1";

Scrap this.

@@ +28,5 @@
>  const SAVE_PER_SITE_PREF = LAST_DIR_PREF + ".savePerSite";
>  const PBSVC_CID = "@mozilla.org/privatebrowsing;1";
>  const nsILocalFile = Components.interfaces.nsILocalFile;
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;

Are these necessary?

@@ +50,1 @@
>          if (aData == "enter")

This is now incorrect. There's no aData for last-pb-context-exited; it only fires in the exit case now.
Attachment #640205 - Flags: feedback?(josh)
Attachment #640205 - Flags: feedback?(ehsan)
What this does :
This patch is based on the synchronous approach (similar to the original xpcshell tests).

Positives :
This patch passed without the Part 5(v2) applied, i.e. without any code changes.

Negatives :
This patch fails when running with Part 5(v2) applied. I am not able to figure out the source of the problems. The line which is failing is :

|is(gDownloadLastDir.file.path, tmpDir.path, "")| .

It's after the first time, that PB mode is turned on. The error is this :

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | Exception thrown at chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js:104 - TypeError: gDownloadLastDir.file is null

Ideas for a solution :
None so far.
Attachment #638450 - Attachment is obsolete: true
Attachment #640207 - Flags: feedback?(josh)
Attachment #640207 - Flags: feedback?(ehsan)
Try making the fixes I requested for patch 5 and run it again.
Note : The description below is quite similar to the previous(Part 4(v2)), but what's written has actually been thought about and not just copy pasted from previous case (as it might seem) .

What this does:
This patch is based on the synchronous approach (similar to the original xpcshell tests).

Positives :
This patch passed without the Part 5(v2) applied, i.e. without any code changes.

Negatives :
This patch fails when running with Part 5(v2) applied. I am not able to figure out the source of the problems. The line which is failing is :

is(gDownloadLastDir.file.path, tmpDir.path, "")

It's after the first time, that PB mode is turned on. The error is this :

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | Exception thrown at chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js:104 - TypeError: gDownloadLastDir.file is null

Ideas for a solution :
The problem here is the exactly the same as Part 4(v2), which leads me to think that there is (most probably) a problem with Part 5(v2), but something which I can't figure out. So, if we figure out the cause for Part 4(v2), this should be solved also :)
Attachment #638359 - Attachment is obsolete: true
Attachment #640210 - Flags: feedback?(josh)
@Josh, sorry didn't see the comments. Let me fix the comments in your patch and then re-run the tests. Thanks.
Comment on attachment 640207 [details] [diff] [review]
Part 4 - Coversion of test_DownloadLastDirWithCPS.js to mochitest(v2)

> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | Exception thrown at chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js:104 - TypeError: gDownloadLastDir.file is null

You should use this warning message to debug what's going wrong.  It seems like the getFile function is returning null when you don't expect it to.
Attachment #640207 - Flags: feedback?(ehsan)
Hurdles :
Since "last-pb-context-exited" didn't have "enter" observer, the correct behaviour wasn't being executed.

What this Path does:
This patch uses a boolean flag, to track this.

Positives:
All tests pass. \o/

Negatives:
None so far.
Attachment #640205 - Attachment is obsolete: true
Attachment #640337 - Flags: review?(ehsan)
All tests passing.
Attachment #640210 - Attachment is obsolete: true
Attachment #640210 - Flags: feedback?(josh)
Attachment #640339 - Flags: review?(ehsan)
Attachment #640207 - Attachment is obsolete: true
Attachment #640207 - Flags: feedback?(josh)
Attachment #638309 - Attachment is obsolete: true
Attachment #638303 - Attachment is obsolete: true
All tests passing.
Attachment #640340 - Flags: review?(ehsan)
Comment on attachment 640337 [details] [diff] [review]
Part 5 - Actual Changes to the JSM's(v3)

You'll want to clean up the last-pb-context-exited observer to get rid of the code that isn't being run any longer.
@Josh, you mean :

if (aData == "enter")
           gDownloadLastDirFile = readLastDirPref();

or anything else besides this also ?
It is very weird that all the tests are passing locally and during the try run, they fail. I asked dzbarsky and he said there might be some problem with the try in loading revisions.

Guys, any ideas about alternate methods of proof of correctness of patch like maybe screenshots or any solution for this "try" situation ?
Comment on attachment 640337 [details] [diff] [review]
Part 5 - Actual Changes to the JSM's(v3)

I don't really understand what this patch tries to do with the gDownloadLastDirFileStatus variable.

Here's what gDownloadLastDir should do:

* When reading the file property (inside getFile), check to see if the window that it belongs into is private.  If it is, return the value of gDownloadLastDirFile.  If it's not, then return readLastDirPref().
* When setting the file property (inside setFile), check to see if the window that it belongs to is private.  If it is, set the value of gDownloadLastDirFile, otherwise set the pref.

And once last-pb-context-exited is received, you just need to set gDownloadLastDirFile to null.

The effect of this patch would be that private and non-private windows will have different last downloaded directories.  The last downloaded directory for private windows is stored in gDownloadLastDirFile, and the one for non-private windows will be stored in the prefs database.  The code to handle last-pb-context-exited is only needed that if you close all of the private windows and then open a new private window, it will not inherit the download last dir of the last private window.
Attachment #640337 - Flags: review?(ehsan) → review-
Comment on attachment 640343 [details] [diff] [review]
Part 1-Conversion of test_privatebrowsing_downloadLastDir.js to mochitest(v3)

Review of attachment 640343 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments below.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadLastDir.js
@@ +49,5 @@
> +
> +  prefs.setComplexValue("lastDir", Ci.nsIFile, tmpDir);
> +  MockFilePicker.returnFiles = [file1];
> +  let file = launcherDialog.promptForSaveToFile(launcher, context, null, null, null);
> +  ok(!!file, "");

Here and elsewhere in the tests, please add suitable text messages as the last arguments of test functions.  Having empty strings makes it extremely difficult to read the logs of test runs and figure out what they do.  A good example here would be: "promptForSaveToFile correctly returned a file".

@@ +85,5 @@
> +  // gDownloadLastDir should be usable after leaving the private browsing mode
> +  is(gDownloadLastDir.file.path, dir3.path, "");
> +
> +  // cleanup
> +  registerCleanupFunction(function () {

You want to do this right after you create the file and directory objects above.  Otherwise if something throws above, for example, you have not registered your cleanup function in time.

@@ +90,5 @@
> +    Services.prefs.clearUserPref("browser.privatebrowsing.keep_current_session");
> +    [dir1, dir2, dir3].forEach(function(dir) dir.remove(true));
> +    MockFilePicker.cleanup();
> +    delete gDownloadLastDir;
> +    delete FileUtils;

You shouldn't need to delete these variables manually.

@@ +91,5 @@
> +    [dir1, dir2, dir3].forEach(function(dir) dir.remove(true));
> +    MockFilePicker.cleanup();
> +    delete gDownloadLastDir;
> +    delete FileUtils;
> +    });

Nit: please fix the indentation.
Attachment #640343 - Flags: review?(ehsan) → review+
Comment on attachment 640341 [details] [diff] [review]
Part 2 - Conversion of test_privatebrowsing_downloadLastDir_c.js to mochitest(v3)

Review of attachment 640341 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but please address my comments from the first part here as well.
Attachment #640341 - Flags: review?(ehsan) → review+
Comment on attachment 640340 [details] [diff] [review]
Part 3 - Conversion of test_DownloadLastDir.js to mochitest(v3)

Review of attachment 640340 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments from the first part addressed here as well.
Attachment #640340 - Flags: review?(ehsan) → review+
Comment on attachment 640339 [details] [diff] [review]
Part 4 - Coversion of test_DownloadLastDirWithCPS.js to mochitest(v3)

Review of attachment 640339 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the below, and also proper descriptions as the last argument to the test functions.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js
@@ +37,5 @@
> +  let dir1 = newDir();
> +  let dir2 = newDir();
> +  let dir3 = newDir();
> +  try {
> +    { // set up last dir                                                                  }

Nit: this trailing } here is very confusing.  :-)

@@ +209,5 @@
> +      gDownloadLastDir.setFile(uri3, null);
> +      is(gDownloadLastDir.getFile(uri3), null, "");
> +    }
> +  } finally {
> +      registerCleanupFunction(function () {

No need to call registerCleanupFunction here, as this is all happening in the finally block which gets run if something down here throws.  :-)  Just do the cleanup here directly.

@@ +215,5 @@
> +        dir2.remove(true);
> +        dir3.remove(true);
> +        Services.prefs.clearUserPref("browser.download.lastDir.savePerSite");
> +        delete gDownloadLastDir;
> +        delete FileUtils;

Again, no need to delete these variables here.
Attachment #640339 - Flags: review?(ehsan) → review+
(In reply to Ehsan Akhgari [:ehsan] from comment #57)
> Comment on attachment 640337 [details] [diff] [review]
> Part 5 - Actual Changes to the JSM's(v3)
> 
> I don't really understand what this patch tries to do with the
> gDownloadLastDirFileStatus variable.
> 
> Here's what gDownloadLastDir should do:
> 
> * When reading the file property (inside getFile), check to see if the
> window that it belongs into is private.  If it is, return the value of
> gDownloadLastDirFile.  If it's not, then return readLastDirPref().
> * When setting the file property (inside setFile), check to see if the
> window that it belongs to is private.  If it is, set the value of
> gDownloadLastDirFile, otherwise set the pref.
> 
> And once last-pb-context-exited is received, you just need to set
> gDownloadLastDirFile to null.
> 
> The effect of this patch would be that private and non-private windows will
> have different last downloaded directories.  The last downloaded directory
> for private windows is stored in gDownloadLastDirFile, and the one for
> non-private windows will be stored in the prefs database.  The code to
> handle last-pb-context-exited is only needed that if you close all of the
> private windows and then open a new private window, it will not inherit the
> download last dir of the last private window.

Consider these steps:
1) open a single private window
2) close the private window
3) open another private window
4) save a file

Suddenly gDownloadLastDirFile is null and can't be used. This is why Saurabh introduced the status variable, to replicate the effect of the existing private-browsing enter observer which updates gDownloadLastDirFile to the current value of the preference.
On that subject, I'd like to see the test results after we:
* update the boolean value inside browser:purge-session-history
* initialize it to true (since gDownloadLastDirFile has a real value at the start)
* update the last-pb-context-exited observer to remove any checks related to aData and only run the exit code
@Ehsan, you are correct I don't need to delete gDownloadLastDir in the cleanup, since it's scope is not global. But I'll have to delete FileUtils, since it's injected into the global scope and not deleting in the cleanup functions will cause leakage which was causing the tests to fail, hence I did that.
Attachment #640337 - Attachment is obsolete: true
Attachment #641033 - Flags: review?(ehsan)
Attachment #640340 - Attachment is obsolete: true
Attachment #641044 - Flags: review?(ehsan)
Well there seems to be a piece of magic code (unexplained code) here. What I observed is that the latest Part 5(v4) still causes Part 2 to fail, while Part 3 and Part 4 were passing happily. I and Josh, wondered and it seems that keeping everything as it is in "v4" of all the parts and just simply initializing gDownloadLastDirFileStatus to "false" solves all the problems, which is ... mysterious (because of no explanation) and satisfying (because it makes the tests pass :) ).

So if anyone has any explanations/ideas for this behavior, please enlighten me.

Thanks.
(In reply to Saurabh Anand [:sawrubh] from comment #64)
> @Ehsan, you are correct I don't need to delete gDownloadLastDir in the
> cleanup, since it's scope is not global. But I'll have to delete FileUtils,
> since it's injected into the global scope and not deleting in the cleanup
> functions will cause leakage which was causing the tests to fail, hence I
> did that.

OK sounds good.
(In reply to Josh Matthews [:jdm] from comment #62)
> (In reply to Ehsan Akhgari [:ehsan] from comment #57)
> > Comment on attachment 640337 [details] [diff] [review]
> > Part 5 - Actual Changes to the JSM's(v3)
> > 
> > I don't really understand what this patch tries to do with the
> > gDownloadLastDirFileStatus variable.
> > 
> > Here's what gDownloadLastDir should do:
> > 
> > * When reading the file property (inside getFile), check to see if the
> > window that it belongs into is private.  If it is, return the value of
> > gDownloadLastDirFile.  If it's not, then return readLastDirPref().
> > * When setting the file property (inside setFile), check to see if the
> > window that it belongs to is private.  If it is, set the value of
> > gDownloadLastDirFile, otherwise set the pref.
> > 
> > And once last-pb-context-exited is received, you just need to set
> > gDownloadLastDirFile to null.
> > 
> > The effect of this patch would be that private and non-private windows will
> > have different last downloaded directories.  The last downloaded directory
> > for private windows is stored in gDownloadLastDirFile, and the one for
> > non-private windows will be stored in the prefs database.  The code to
> > handle last-pb-context-exited is only needed that if you close all of the
> > private windows and then open a new private window, it will not inherit the
> > download last dir of the last private window.
> 
> Consider these steps:
> 1) open a single private window
> 2) close the private window
> 3) open another private window
> 4) save a file
> 
> Suddenly gDownloadLastDirFile is null and can't be used. This is why Saurabh
> introduced the status variable, to replicate the effect of the existing
> private-browsing enter observer which updates gDownloadLastDirFile to the
> current value of the preference.

Seems to me like we can just read the pref value if we need gDownloadLastDirFile and it's null, right?
(In reply to Ehsan Akhgari [:ehsan] from comment #72)
> (In reply to Josh Matthews [:jdm] from comment #62)
> > (In reply to Ehsan Akhgari [:ehsan] from comment #57)
> > > Comment on attachment 640337 [details] [diff] [review]
> > > Part 5 - Actual Changes to the JSM's(v3)
> > > 
> > > I don't really understand what this patch tries to do with the
> > > gDownloadLastDirFileStatus variable.
> > > 
> > > Here's what gDownloadLastDir should do:
> > > 
> > > * When reading the file property (inside getFile), check to see if the
> > > window that it belongs into is private.  If it is, return the value of
> > > gDownloadLastDirFile.  If it's not, then return readLastDirPref().
> > > * When setting the file property (inside setFile), check to see if the
> > > window that it belongs to is private.  If it is, set the value of
> > > gDownloadLastDirFile, otherwise set the pref.
> > > 
> > > And once last-pb-context-exited is received, you just need to set
> > > gDownloadLastDirFile to null.
> > > 
> > > The effect of this patch would be that private and non-private windows will
> > > have different last downloaded directories.  The last downloaded directory
> > > for private windows is stored in gDownloadLastDirFile, and the one for
> > > non-private windows will be stored in the prefs database.  The code to
> > > handle last-pb-context-exited is only needed that if you close all of the
> > > private windows and then open a new private window, it will not inherit the
> > > download last dir of the last private window.
> > 
> > Consider these steps:
> > 1) open a single private window
> > 2) close the private window
> > 3) open another private window
> > 4) save a file
> > 
> > Suddenly gDownloadLastDirFile is null and can't be used. This is why Saurabh
> > introduced the status variable, to replicate the effect of the existing
> > private-browsing enter observer which updates gDownloadLastDirFile to the
> > current value of the preference.
> 
> Seems to me like we can just read the pref value if we need
> gDownloadLastDirFile and it's null, right?

Nope. There are tests that break in that case. I thought about Saurabh's comment 71, and realized that initializing the status variable as false is not magic but correct. The initial preference when the script is run is not the value we want to end up with when entering PB mode, so we should wait to read the pref until the first request in PB mode.
This latest patch now has all the tests passing finally, with acceptance from Ehsan and Josh \o/.

@Ehsan I think we are good to go here. If you want a try run, tell me , but I can't promise it will work, because last time, some weird issues came up, but I can certainly "try".
Attachment #641033 - Attachment is obsolete: true
Attachment #641033 - Flags: review?(ehsan)
Attachment #641229 - Flags: review?(ehsan)
@Ehsan, one more thing that I changed in Part 5(v5) is that I changed the behavior of "getTargetFile" to cache the import of DownloadLastDir.jsm and now I'm always importing it into a local object.
Please do a try run; I want to feel confident that this won't be immediately backed out.
Ah yes, the build fails on OS X and Windows. You need to rename the new browser_privatebrowsing_DownloadLastDir.js to something like browser_privatebrowsing_DownloadLastDir_toggle.js, since there's already a browser_privatebrowsing_downloadLsatDir.js in the folder (note the case difference).
Furthermore, the failing mochitest-browser-chrome tests suggests this is not ready for review yet.

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | LastDir should be cleared - Got /tmp/testdir-1, expected /tmp/testdir
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadLastDir_c.js | File picker should start with LastDir set before entering PB mode - Got /tmp/testdir-1, expected /tmp/testdir
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_pageinfo.js | application timed out after 330 seconds with no output
(In reply to Josh Matthews [:jdm] from comment #73)
> Nope. There are tests that break in that case. I thought about Saurabh's
> comment 71, and realized that initializing the status variable as false is
> not magic but correct. The initial preference when the script is run is not
> the value we want to end up with when entering PB mode, so we should wait to
> read the pref until the first request in PB mode.

Hmm, not sure which comment you're referring to Josh (comment 71 was written by me!), but I'm still not convinced.  With the current patch part 5, I think that gDownloadLastDirFileStatus is false every time that gDownloadLastDirFile is null, and the only time when gDownloadLastDirFile can be null and gDownloadLastDirFileStatus be true is if you pass a null file to setFile, which would be weird.  So I don't think this is actually solving any problems, since it's practically a copy of the existing state we're maintaining in gDownloadLastDirFile.  Am I missing something?
Perhaps not. I think I was getting confused by some of the tests, but I find your reasoning convincing.
Comment on attachment 641034 [details] [diff] [review]
Part 1 - Conversion of test_privatebrowsing_downloadLastDir.js to mochitest(v4)

Clearing the requests for now.
Attachment #641034 - Flags: review?(ehsan)
Attachment #641229 - Flags: review?(ehsan)
Attachment #641144 - Flags: review?(ehsan)
Attachment #641044 - Flags: review?(ehsan)
Attachment #641043 - Flags: review?(ehsan)
So I've learnt from the previous try run, that tests were passing individually but not when the entire "privatebrowsing/test/browser/" suite is run together. Then that tests suite seems to get stuck at the call to gBrowser.removeCurrentTab<http://dxr.lanedo.com/mozilla-central/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_pageinfo.js.html#62> saying that it's not a function and it times out.

So I tested the 4 parts (Part 1 to 4) individually, and they passed but when I ran them with the above mentioned test suite, it failed ! This was when I had not applied any code changes (i.e. Part 5), which make me think there is either a fault with the porting of the xpcshell tests. I think it's one of the two reasons :

a) The tests need to be switching of PB modes needs to be synchronous, but I had discussed this with Josh and Ehsan and we thought that it shouldn't be necessary.
b) Maybe the tests need to updated. I mean, there might be something which was correct earlier, but maybe it's not in the new, mochitests case.

Since I don't have any code changes applied, hence I am assuming there is a problem with the tests. Currently am trying to check what the tests are doing and where they might be wrong.

PS : Josh, the suggestion you gave about using |getWeakReference| in DownloadLastDir, I will try that after I get the tests to pass, while running the entire suite, without any code changes.
One thing to investigate would be what happens if you get rid of the bits in the tests that override ContentAreaUtils.stringBundle and validateFileName.
@Josh,Ehsan sorry for my delayed activity on this bug for the last couple of days. I'm back now. I'll be working actively on this bug now.

Note : All the tests below were run without any code changes applied.
Now I first tested each test one by one, to see which one was the first to fail while running entire the test suite. So "browser_privatebrowsing_downloadLastDir.js" (Part 1) passed the complete test suite :).

Then I tested "browser_privatebrowsing_downloadLastDir_c.js" (Part 2) and the test suite timed out. Mind you, the test in it self passed, it was "browser_privatebrowsing_pageinfo.js" which timed out (like before). Then I commented out the parts of Part 2 which were overriding ContentAreaUtils.stringBundle and validateFileName and this is the error I got :

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadLastDir_    c.js | Exception thrown - [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.GetStringFromName]"  nsres    ult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://global/content/contentAreaUtils.js :: getTargetFile :: line 573"  data: no]

which is basically this call :

fp.init(window, ContentAreaUtils.stringBundle.GetStringFromName(titleKey),
          Components.interfaces.nsIFilePicker.modeSave);

in getTargetFile() inside contentAreaUtils.js. I am not understanding much but would like to debug this tonight when you guys come online. Meanwhile, why were we originally overriding this stringBundle function in the test, is it so that we don't open a real filepicker (I have already removed that |makeFilePicker| and am using MockFilePicker in its place) ?
Hmm, calling Services.obs.notifyObservers(null, "quit-application", null); in the test is wrong.  This is the notification which happens when Firefox is exiting, and running it in the middle of browser chrome tests is a very bad idea, as it may cause some components cleanup their internal state, etc.  Note that this was fine in the xpcshell test because 1) xpcshell doesn't automatically dispatch this when exiting, so it was needed to clean things up, and 2) each xpcshell test is run in a separate shell program which starts up, runs the single test and shuts down, so xpcshell tests cannot have this kind of negative impact over the tests following them.
(In reply to Saurabh Anand [:sawrubh] from comment #85)
> which is basically this call :
> 
> fp.init(window, ContentAreaUtils.stringBundle.GetStringFromName(titleKey),
>           Components.interfaces.nsIFilePicker.modeSave);
> 
> in getTargetFile() inside contentAreaUtils.js. I am not understanding much
> but would like to debug this tonight when you guys come online. Meanwhile,
> why were we originally overriding this stringBundle function in the test, is
> it so that we don't open a real filepicker (I have already removed that
> |makeFilePicker| and am using MockFilePicker in its place) ?

I don't remember why we were overriding the string bundle.  It might have been because the string bundle service is not available in the xpcshell test suite.  Try taking it out, and if the test passes both with and without your code changes, leave it out.  :-)
@Ehsan, Thanks for the suggestion, it worked and Part 2 is passing !

Note: I've moved to Part 3 and this is the error I'm getting :

gDownloadLastDir.file should be null to start with - Got [xpconnect wrapped nsILocalFile], expected null

in this line :

   is(gDownloadLastDir.file, null, "gDownloadLastDir.file should be null to start with");

I and Josh thought this might be a cleanup issue.

@Josh, you said that theoretically last-pb-context-exited is triggered at the end of the test, which resets it to null, but wouldn't that have happened, had I applied my Part 5(where I do such things such as setting the observer for last-pb-context-exited and resetting things to null), but without my code changes, are such things supposed to happen ?

Also you suggested that I should clear the preference at the end of the part 2 test, but something like this is already happening in registerCleanupFunction via this call :

    Services.prefs.clearUserPref("browser.privatebrowsing.keep_current_session");

Did you mean something else, or did you want me to do this clearing, outside registerCleanupFunction() , at the end of Part 2 ?
I actually meant you should try clearing the lastDir pref. The next test will have whatever the previous test last set it to, which would cause the failure you report. You can ignore my comment about last-pb-context-exited for now, I was just confused.
@Josh, I added the cleanup code to clear the lastDir pref in Part 2 and now all the tests are passing the entire tests suite. Do you think I should add this "lastDir clearing" cleanup code to the registerCleanupFunctions of all the tests, to be safe. Currently just adding it to Part 2 is making all tests pass, but I'm suggesting this, just to be future proof.

Next: Checking whether the entire test suite is passing _with_ the code changes (Part 5).
So there is this one failure, when running the entire test suite with the code changes and 4 tests, all applied and it is :

File picker should start with LastDir set before entering PB mode - Got /tmp/testdir-1, expected /tmp/testdir

in this line :

  is(MockFilePicker.displayDirectory.path, dir1.path, "File picker should start with LastDir set before entering PB mode")

in browser_privatebrowsing_downloadLastDir_c.js . I have removed both the status variable and am checking gDownloadLastDirFile for null and reading the pref, when required plus I have also done that |getWeakReference| change which Josh proposed.

If you want I can post an updated patch for the code changes here.
(In reply to comment #90)
> @Josh, I added the cleanup code to clear the lastDir pref in Part 2 and now all
> the tests are passing the entire tests suite. Do you think I should add this
> "lastDir clearing" cleanup code to the registerCleanupFunctions of all the
> tests, to be safe.

Yes!
(In reply to Saurabh Anand [:sawrubh] from comment #91)
> If you want I can post an updated patch for the code changes here.

Please do that.  Also, see if you can debug this error as well.  It might be caused by the fact that a tmp directory is not correctly deleted at the end of a test (since the "-1" prefix is probably added by createUniq as /tmp/testdir existed when it was called to create a tmp dir.)
This is a new Part that I introduced, in order to make this bug more modular. Without this part pushed, the tests will work without any code changes. After pushing this , the tests become compatible so that code changes can be made and the tests will work as they are.
Attachment #641229 - Attachment is obsolete: true
So as suggested by Josh, adding |gDownloadLastDir.setFile(null, null)| inside PB mode in the registerCleanupFunction in the tests, does make all the tests pass.

@Ehsan, waiting for your validation, on this action. Currently, I've just added this cleanup code to Part 1 and Part 2 and all the tests seem to pass. If you want I can upload the patch with this addition here and also do a try run.

/me can't wait any longer.
I'm attaching the updated patches anyhow, and doing a try run, so that when you guys get active in the morning, you have all the information at hand.
Added the cleanup code to make tests pass. Refer comment 100.
Attachment #643402 - Attachment is obsolete: true
Nothing special. Just makes sure the earlier patch was not getting bitrotted, since when applying it, it did a "fuzzy" merge.
Attachment #643409 - Attachment is obsolete: true
Trivial change. Fixed the semicolon, on the line importing PrivateBrowsingUtils module in DownloadLastDir.jsm.
Attachment #643410 - Attachment is obsolete: true
Did you try running all the privatebrowsing/ tests with the page 2 test waiting to start until the last-pb-context-exited notification like I suggested? I don't want to accept this workaround until we're sure that the reason is just a delayed (not missing) notification.
(In reply to comment #107)
> Did you try running all the privatebrowsing/ tests with the page 2 test waiting
> to start until the last-pb-context-exited notification like I suggested? I
> don't want to accept this workaround until we're sure that the reason is just a
> delayed (not missing) notification.

Agreed.  If you can verify this I'm fine with it as well.
I verified Josh's theory, and the test, after adding that observer for |last-pb-context| and removing any bits that explicitly called |setFile|, passed the entire test suite.

This is the explanation, which justified the workaround :
During xpcshell tests, after each test ended the shell was exit and hence the file variable was reset(which was desired), but with the mochitests, not exiting in between tests, this was supposed to happen based on |last-pb-context-exited| event being fired, which is supposed to correspond to leaving PB mode, but it doesn't always happen at the same time. However if we wait long enough, the notification, will be fired, this is what was confirmed by the test today.

So the workaround for ensuring that the file is reset at the correct time, is that we explicitly reset it during the cleanup after the individual tests finish, and this is what's done in the latest patch.

@Josh, would you like to add anything here ?
Nope.
If this is ready for review, please mark it as such.  Thanks!
Attachment #643404 - Attachment is obsolete: true
Attachment #643964 - Flags: review?(ehsan)
Attachment #643795 - Attachment is obsolete: true
Attachment #643968 - Flags: review?(ehsan)
Removed the |getWeakReference| bits since they were not required.
Attachment #643796 - Attachment is obsolete: true
Attachment #643969 - Flags: review?(ehsan)
Comment on attachment 643960 [details] [diff] [review]
Part 1 - Conversion of test_privatebrowsing_downloadLastDir.js to mochitest(v7)

Review of attachment 643960 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below addressed.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadLastDir.js
@@ +51,5 @@
> +    [dir1, dir2, dir3].forEach(function(dir) dir.remove(true));
> +    MockFilePicker.cleanup();
> +    pb.privateBrowsingEnabled = true;
> +    gDownloadLastDir.setFile(null, null);
> +    pb.privateBrowsingEnabled = false;

Switching the PB mode on and off in order to clear a flag is not really great.  I'd prefer if you would add a new method to gDownloadLastDir named cleanupPrivateFile which does this, and just call that from the tests.

Please do this in the other tests as well.
Attachment #643960 - Flags: review?(ehsan) → review+
Comment on attachment 643962 [details] [diff] [review]
Part 2 - Conversion of test_privatebrowsing_downloadLastDir_c.js to mochitest(v7)

Review of attachment 643962 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadLastDir_c.js
@@ +60,5 @@
> +  // Overwrite stringBundle to return an object masquerading as a string bundle
> +  delete ContentAreaUtils.stringBundle;
> +  ContentAreaUtils.stringBundle = {
> +    GetStringFromName: function() ""
> +  };

So, why is this necessary?  I almost convinced myself that it's not.  What happens if you take it out?
Comment on attachment 643964 [details] [diff] [review]
Part 3 - Conversion of test_DownloadLastDir.js to mochitest(v6)

Review of attachment 643964 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comment about switching PB on and off.
Attachment #643964 - Flags: review?(ehsan) → review+
Comment on attachment 643966 [details] [diff] [review]
Part 4 - Coversion of test_DownloadLastDirWithCPS.js to mochitest(v6)

Review of attachment 643966 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the same comment.
Attachment #643966 - Flags: review?(ehsan) → review+
Attachment #643968 - Flags: review?(ehsan) → review+
Attachment #643969 - Flags: review?(ehsan) → review+
@Ehsan, so I tried taking out the bit of code which was overriding the stringBundle as you suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=722995#c119 , but the test failed with this error :

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadLastDir_c.js | Exception thrown - [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.GetStringFromName]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://global/content/contentAreaUtils.js :: getTargetFile :: line 573"  data: no]

Note: I had just commented out the code responsible for overriding the stringBundle, the code which was overriding validateFileName was still there.
(In reply to Saurabh Anand [:sawrubh] from comment #122)
> @Ehsan, so I tried taking out the bit of code which was overriding the
> stringBundle as you suggested in
> https://bugzilla.mozilla.org/show_bug.cgi?id=722995#c119 , but the test
> failed with this error :
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/components/privatebrowsing/test/
> browser/browser_privatebrowsing_downloadLastDir_c.js | Exception thrown -
> [Exception... "Component returned failure code: 0x80004005
> (NS_ERROR_FAILURE) [nsIStringBundle.GetStringFromName]"  nsresult:
> "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
> chrome://global/content/contentAreaUtils.js :: getTargetFile :: line 573" 
> data: no]
> 
> Note: I had just commented out the code responsible for overriding the
> stringBundle, the code which was overriding validateFileName was still there.

Huh, ok, then don't worry about it!
Comment on attachment 643962 [details] [diff] [review]
Part 2 - Conversion of test_privatebrowsing_downloadLastDir_c.js to mochitest(v7)

Review of attachment 643962 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comment about switching PB on and off addressed.
Attachment #643962 - Flags: review?(ehsan) → review+
Carries r=ehsan.
Attachment #643969 - Attachment is obsolete: true
@checkin-buddies, Please make sure that you land in order that is, Part 1 first, then Part 2 and so on. Only then will these "apply" or merge smoothly with each other.

Thanks.
Sorry, but I had to back this out for causing frequent linux32 mochitest-other orange.
https://hg.mozilla.org/mozilla-central/rev/a3e1c960433b

Here's an example of the failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=13739957&tree=Mozilla-Inbound

TEST-START | chrome://mochitests/content/browser/image/test/browser/browser_bug666317.js
TEST-INFO | chrome://mochitests/content/browser/image/test/browser/browser_bug666317.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 to be declared in the document or in the transfer protocol." {file: "data:text/html,<html><body><img%20id="testImg"%20src="http://example.com/browser/image/test/browser/big.png"></body></html>" line: 0}]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/image/test/browser/browser_bug666317.js | an unexpected uncaught JS exception reported through window.onerror - NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIDOMCanvasRenderingContext2D.drawImage] at chrome://mochitests/content/browser/image/test/browser/browser_bug666317.js:27
Stack trace:
    JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 994
    native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

TEST-INFO | chrome://mochitests/content/browser/image/test/browser/browser_bug666317.js | Console message: [JavaScript Error: "Image corrupt or truncated: http://example.com/browser/image/test/browser/big.png" {file: "http://example.com/browser/image/test/browser/big.png" line: 0}]
TEST-INFO | chrome://mochitests/content/browser/image/test/browser/browser_bug666317.js | Console message: [JavaScript Error: "NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIDOMCanvasRenderingContext2D.drawImage]" {file: "chrome://mochitests/content/browser/image/test/browser/browser_bug666317.js" line: 27}]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/image/test/browser/browser_bug666317.js | Test timed out


This also may have also been causing pdf.js test OOMs occasionally (unfortunately, with no usable stack):
https://tbpl.mozilla.org/php/getParsedLog.php?id=13739941&tree=Mozilla-Inbound

TEST-START | chrome://mochitests/content/browser/browser/extensions/pdfjs/test/browser_pdfjs_main.js
TEST-PASS | chrome://mochitests/content/browser/browser/extensions/pdfjs/test/browser_pdfjs_main.js | pdf handler defaults to always-ask is false
TEST-PASS | chrome://mochitests/content/browser/browser/extensions/pdfjs/test/browser_pdfjs_main.js | pdf handler defaults to internal
TEST-INFO | chrome://mochitests/content/browser/browser/extensions/pdfjs/test/browser_pdfjs_main.js | Pref action: 3
out of memory
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/extensions/pdfjs/test/browser_pdfjs_main.js | Exited with code 1 during test run
INFO | automation.py | Application ran for: 0:15:02.769571
INFO | automation.py | Reading PID log: /tmp/tmpY0dHyopidlog
==> process 2605 launched child process 2656
INFO | automation.py | Checking for orphan process with PID: 2656
Downloading symbols from: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux/1342852782/firefox-17.0a1.en-US.linux-i686.crashreporter-symbols.zip
PROCESS-CRASH | chrome://mochitests/content/browser/browser/extensions/pdfjs/test/browser_pdfjs_main.js | application crashed (minidump found)
Crash dump filename: /tmp/tmp7UGmfJ/minidumps/70129604-fa1d-f053-324445c5-20405e55.dmp
Operating system: Linux
                  0.0.0 Linux 2.6.31.5-127.fc12.i686.PAE #1 SMP Sat Nov 7 21:25:57 EST 2009 i686
CPU: x86
     GenuineIntel family 6 model 23 stepping 10
     2 CPUs

Crash reason:  SIGSEGV
Crash address: 0x0

Thread 16 (crashed)
 0  libmozalloc.so!mozalloc_abort [mozalloc_abort.cpp : 23 + 0x0]
    eip = 0x00d00eed   esp = 0xa94fefe0   ebp = 0xa94feff8   ebx = 0x00d01120
    esi = 0x00c59844   edi = 0x0018db30   eax = 0x0000000a   ecx = 0x00000001
    edx = 0x00c5a32c   efl = 0x00010246
    Found by: given as instruction pointer in context
 1  libmozalloc.so!mozalloc_handle_oom [mozalloc_oom.cpp : 27 + 0xe]
    eip = 0x00d00f32   esp = 0xa94ff000   ebp = 0xa94ff018   ebx = 0x00d01120
    esi = 0x0031b660   edi = 0x0018db30
    Found by: call frame info
 2  libmozalloc.so!moz_xmalloc [mozalloc.cpp : 56 + 0x8]
    eip = 0x00d00b6d   esp = 0xa94ff020   ebp = 0xa94ff038   ebx = 0x00d01120
    esi = 0x0031b660   edi = 0x0018db30
    Found by: call frame info
 3  libxul.so + 0x87d509
    eip = 0x0157f50a   esp = 0xa94ff040   ebp = 0xa94ff068   ebx = 0x024dda50
    esi = 0xa94ff098   edi = 0x0018db30
    Found by: call frame info
 4  libxul.so + 0x888174
    eip = 0x0158a175   esp = 0xa94ff070   ebp = 0xa94ff0a8
    Found by: previous frame's frame pointer
 5  libxul.so + 0x87c2d5
    eip = 0x0157e2d6   esp = 0xa94ff0b0   ebp = 0xa94ff0c8
    Found by: previous frame's frame pointer
 6  libxul.so + 0x885c4e
    eip = 0x01587c4f   esp = 0xa94ff0d0   ebp = 0xa94ff118
    Found by: previous frame's frame pointer
 7  libxul.so + 0x885d6e
    eip = 0x01587d6f   esp = 0xa94ff120   ebp = 0xa94ff168
    Found by: previous frame's frame pointer
 8  libxul.so + 0x895b33
    eip = 0x01597b34   esp = 0xa94ff170   ebp = 0xa94ff1c8
    Found by: previous frame's frame pointer
 9  libxul.so + 0x895d00
    eip = 0x01597d01   esp = 0xa94ff1d0   ebp = 0xa94ff208
    Found by: previous frame's frame pointer
10  libxul.so + 0x895d91
    eip = 0x01597d92   esp = 0xa94ff210   ebp = 0xa94ff238
    Found by: previous frame's frame pointer
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla17 → ---
There also appears to be random orange in the new test added by this bug.

https://tbpl.mozilla.org/php/getParsedLog.php?id=13743196&tree=Mozilla-Inbound

TEST-PASS | chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | uri1 should return dir2 inside PB mode
TEST-PASS | chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | LastDir should point to dir1 outside PB mode
TEST-PASS | chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | uri1 should return dir1 outside PB mode
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | LastDir should be cleared - Got /tmp/testdir-1, expected /tmp/testdir
Stack trace:
    JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 438
    JS frame :: chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js :: test :: line 167
    JS frame :: chrome://mochikit/content/browser-test.js :: Tester_execTest :: line 338
    JS frame :: chrome://mochikit/content/browser-test.js :: <TOP_LEVEL> :: line 279
    native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

TEST-PASS | chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | uri1 should return dir1


I'm going to strongly recommend retriggering *a lot* of linux moth runs on Try before landing this again.
The reason why DownloadLastDirWithCPS could fail there is due to the last-pb-context-exited notification not firing in time. This is... surprising. It would be useful to put a dump call in the observer when running more tests so we can clarify the behaviour we're seeing in failures.
Sorry, it's been a long time since this has been stuck and am currently having a few other things on my priority, so won't be able to look at this.

Sorry.
Assignee: saurabhanandiit → nobody
No worries, I am actively engaged in fixing it.
Assignee: nobody → josh
Blocks: mobilepb
Generators make this test very pleasant.
Attachment #673348 - Flags: review?(ehsan)
Comment on attachment 673348 [details] [diff] [review]
Make private browsing CPS test wait for private browsing exit events properly.

Review of attachment 673348 [details] [diff] [review]:
-----------------------------------------------------------------

Fancy!
Attachment #673348 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/38183dea71ab
https://hg.mozilla.org/mozilla-central/rev/03cd2ad254cc
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Depends on: 673366
You need to log in before you can comment on or make changes to this bug.