Josh Geenen (:pi) Assignee
	Description • 17 years ago

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.1.14) Gecko/20080604 Firefox/2.0.0.14
Build Identifier: Thunderbird version 3.0a2pre (2008060506)

Mailnews crashes when trying to import upon calling BeginImport() if the field map isn't set when a field map is necessary (for importing CSVs and tab-delimited text files).  For example, if you try to import a CSV file into Thunderbird's Address Book through a script but forget to set the field map and call BeginImport, Thunderbird crashes. 

Suggestion:
Around line 556 of nsImportAddressBooks.cpp - 
http://mxr.mozilla.org/seamonkey/source/mailnews/import/src/nsImportAddressBooks.cpp#556
-
the BeginImport method should check to if the field map is necessary, and, if so, if it was set before continuing.  I have a sample patch that does so and logs an error without crashing Thunderbird.

Reproducible: Always

Steps to Reproduce:
1. Download the first attached file (or make your own CSV export) and place it in your profile directory with the name addressbook.csv (important for the attached code to work)
2. Open Thunderbird and the Error Console
3. Evaluate the code given in the second attachment in the Error Console ***Note: This will crash Thunderbird if the addressbook.csv file is in the profile directory
Actual Results:  
Thunderbird crashes after making a blank address book with the same name as the imported file.

Expected Results:  
The import should quit and log the error and Thunderbird should still be running without adding a blank address book.

Please see the attachments

	Josh Geenen (:pi) Assignee
	Comment 1 • 17 years ago

Save as addressbook.csv in your Thunderbird profile directory

	Josh Geenen (:pi) Assignee
	Comment 2 • 17 years ago

After saving the first attachment, run the long line in TB's error console and TB will crash

	Josh Geenen (:pi) Assignee
	Comment 3 • 17 years ago

A sample patch to fix the problem.

	Serge Gautherie (:sgautherie)
	Comment 4 • 17 years ago

Comment on attachment 323984 [details] [diff] [review]
Sample patch

>-  if (!m_pInterface || !m_pBooks) {
>+  PRBool needsFieldMap = PR_FALSE;
>+  m_pInterface->GetNeedsFieldMap( m_pLocation, &needsFieldMap);

Using |m_pInterface| before checking that it has a value seems odd.
No need for the first space.

>+  if (!m_pInterface || !m_pBooks || !m_pFieldMap && needsFieldMap) {

Don't you miss |()| around |&&| ?

	Josh Geenen (:pi) Assignee
	Comment 5 • 17 years ago

(In reply to comment #4)

> Using |m_pInterface| before checking that it has a value seems odd.
That's right...  m_pLocation as well.  New patch coming soon after I test it.

> >+  if (!m_pInterface || !m_pBooks || !m_pFieldMap && needsFieldMap) {
> 
> Don't you miss |()| around |&&| ?
I think && has a higher precedence, so that was evaluated first, although I should have added parentheses to clarify...

	Josh Geenen (:pi) Assignee
	Comment 6 • 17 years ago

Fixes the mistake mentioned in a previous comment.  It adds a check for m_pLocation, although I'm not sure if that is necessary.

	Mark Banner (:standard8)
	Comment 7 • 17 years ago

Comment on attachment 324093 [details] [diff] [review]
second sample patch

+  if (!m_pInterface || !m_pBooks || !m_pLocation) {

You don't need to check for m_pLocation here, because GetNeedsFieldMap does it...

+  m_pInterface->GetNeedsFieldMap(m_pLocation, &needsFieldMap);
+
+  if(needsFieldMap && !m_pFieldMap) {

So here, you should do something along the lines of:

if (NS_FAILED(m_pInterface->GetNeedsFieldMap(...) ||
    (needsFieldMap && !m_pFieldMap)) {

Note the space between the if and the first bracket.

This way, if GetNeedsFieldMap fails, we'll know about that as well.

	Mark Banner (:standard8)
	Comment 8 • 17 years ago

(In reply to comment #2)
> Created an attachment (id=323983) [details]
> Code to run
> 
> After saving the first attachment, run the long line in TB's error console and
> TB will crash

So I think if you look at what I did in bug 415942 (and also the wiki page at http://wiki.mozilla.org/MailNews:Xpcshell_tests, you'll be able to form this into an automated test pretty easily. You just need to check that when you don't provide a field map that you get an error, and when you do provide one, then the import is successful (we can always add separate checks for the data itself later).

	Josh Geenen (:pi) Assignee
	Comment 9 • 17 years ago

This is a newer patch with a simple test that checks if BeginImport returns false when a field map isn't set.

Regarding the patch, would it be better to add the conditions to the previous if statement? ex.
if (!m_pInterface || !m_pBooks ||
     NS_FAILED(m_pInterface->GetNeedsFieldMap(m_pLocation, &needsFieldMap))||
     (needsFieldMap && !m_pFieldMap)) {

	Mark Banner (:standard8)
	Comment 10 • 17 years ago

Comment on attachment 324351 [details] [diff] [review]
Patch and Test

>Index: mailnews/import/src/nsImportAddressBooks.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/import/src/nsImportAddressBooks.cpp,v
>retrieving revision 1.71
>diff -u -8 -p -r1.71 nsImportAddressBooks.cpp
>--- mailnews/import/src/nsImportAddressBooks.cpp	2 Feb 2008 19:14:04 -0000	1.71
>+++ mailnews/import/src/nsImportAddressBooks.cpp	9 Jun 2008 
>+  PRBool needsFieldMap = PR_FALSE;
>+
>+  if (NS_FAILED(m_pInterface->GetNeedsFieldMap(m_pLocation, &needsFieldMap))||

This needs a space between the )) and ||.

>+     (needsFieldMap && !m_pFieldMap)) {

This needs to be indented by one space more so that the ( lines up with the N of NS_FAILED.

>+    nsImportStringBundle::GetStringByID(IMPORT_ERROR_AB_NOTINITIALIZED,
>+                                        m_stringBundle, error);
>+    SetLogs( success, error, successLog, errorLog);

Please drop the space between ( and success, unfortunately its a common bad mistake in this code module.

>+    *_retval = PR_FALSE;
>+    return( NS_OK);

No need for the brackets here.

>Index: mailnews/import/test/Makefile.in

>+# The Initial Developer of the Original Code is
>+#

You need to add yourself in here (look at other files for examples).

>Index: mailnews/import/test/unit/head_import.js
>+// Import the main scripts that mailnews tests need to set up and tear down
>+do_import_script("mailnews/test/resources/mailDirService.js");
>+

No need for the extra blank line here.

>Index: mailnews/import/test/unit/test_bug_437556.js

>+  var importService = Components.classes["@mozilla.org/import/import-service;1"]
>+                                .getService()
>+                                .QueryInterface(Components.interfaces.nsIImportService);

As we've changed the head files recently, you may not have picked up on it, but you can make this line:

var importService = Cc["@mozilla.org/import/import-service;1"]
                      .getService(Ci.nsIImportService);

>+  var abInterface = module.GetImportInterface("addressbook")
>+                          .QueryInterface(Components.interfaces.nsIImportGeneric);

Here you can use the Ci abbreviation as well.


>+  //BeginImport should return false if the field map isn't set

Please put a space between // and BeginImport

>+  do_check_false(abInterface.BeginImport(null,null,false));

The commas should all have spaces after them.
>+	
>+}

No need for the extra blank lines before and after the } (but we do want a new line to end with ;-) )

	Josh Geenen (:pi) Assignee
	Comment 11 • 17 years ago

In addition to the feedback, I added whitespace to DIRS += test in the import Makefile to make it line up with other parts of the Makefile and changed 2007 to 2008 in "Portions created by the Initial Developer are Copyright (C) 2008" in the new test Makefile.

	Mark Banner (:standard8)
	Comment 12 • 17 years ago

Comment on attachment 324568 [details] [diff] [review]
Patch and Test 2

+  var importService = Cc["@mozilla.org/import/import-service;1"]
+                        .getService()
+                        .QueryInterface(Ci.nsIImportService);

You can merge the getService and QueryInterface into one, just do:

.getService(Ci.nsIImportService);

r=me with that fixed.

	David :Bienvenu
	Comment 14 • 17 years ago

Comment on attachment 324638 [details] [diff] [review]
Patch and Test 3

thx, Josh.

	Mark Banner (:standard8)
	Comment 15 • 17 years ago

Checking in mailnews/import/Makefile.in;
/cvsroot/mozilla/mailnews/import/Makefile.in,v  <--  Makefile.in
new revision: 1.16; previous revision: 1.15
done
Checking in mailnews/import/src/nsImportAddressBooks.cpp;
/cvsroot/mozilla/mailnews/import/src/nsImportAddressBooks.cpp,v  <--  nsImportAddressBooks.cpp
new revision: 1.72; previous revision: 1.71
done
RCS file: /cvsroot/mozilla/mailnews/import/test/Makefile.in,v
done
Checking in mailnews/import/test/Makefile.in;
/cvsroot/mozilla/mailnews/import/test/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/mailnews/import/test/resources/basic_addressbook.csv,v
done
Checking in mailnews/import/test/resources/basic_addressbook.csv;
/cvsroot/mozilla/mailnews/import/test/resources/basic_addressbook.csv,v  <--  basic_addressbook.csv
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/mailnews/import/test/unit/head_import.js,v
done
Checking in mailnews/import/test/unit/head_import.js;
/cvsroot/mozilla/mailnews/import/test/unit/head_import.js,v  <--  head_import.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/mailnews/import/test/unit/test_bug_437556.js,v
done
Checking in mailnews/import/test/unit/test_bug_437556.js;
/cvsroot/mozilla/mailnews/import/test/unit/test_bug_437556.js,v  <--  test_bug_437556.js
initial revision: 1.1
done

	Serge Gautherie (:sgautherie)
	Comment 16 • 17 years ago

I think (a port of) the <nsImportAddressBooks.cpp> part (only) would be wanted on 1.8 branch too, is it not ?

	Mark Banner (:standard8)
	Comment 17 • 17 years ago

(In reply to comment #16)
> I think (a port of) the <nsImportAddressBooks.cpp> part (only) would be wanted
> on 1.8 branch too, is it not ?
> 

Due to the way import works which should be providing a field map when required, I don't think this is really needed. If there's some evidence on the crash servers of crashes, in import due to this, then yes it would be an idea.

	Mark Banner (:standard8)
	Comment 18 • 17 years ago

Reopening, this has been failing on Windows builds:

make[3]: Entering directory `/d/builds/slave/trunk_2k3/mozilla/objdir/mailnews/import/test'
../../../_tests/xpcshell-simple/test_import/unit/test_bug_437556.js: FAIL
../../../_tests/xpcshell-simple/test_import/unit/test_bug_437556.js.log:
>>>>>>>
*** test pending
*** exiting
*** CHECK FAILED: false == true
JS frame :: d:/builds/slave/trunk_2k3/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_throw :: line 99
JS frame :: d:/builds/slave/trunk_2k3/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_check_eq :: line 114
JS frame :: d:/builds/slave/trunk_2k3/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_check_true :: line 118
JS frame :: ../../../_tests/xpcshell-simple/test_import/unit/test_bug_437556.js :: run_test :: line 16
JS frame :: d:/builds/slave/trunk_2k3/mozilla/tools/test-harness/xpcshell-simple/tail.js :: _execute_test :: line 41
JS frame :: d:/builds/slave/trunk_2k3/mozilla/tools/test-harness/xpcshell-simple/execute_test.js :: <TOP_LEVEL> :: line 38
2147500036
*** FAIL ***

Not sure why. WantProgress seems to be returning false. If we disable that check (but not the WantsProgress call) then BeginImport returns false with "No address books were found to import.". I thought it was line endings, but sid0 says he tried fixing those and it was still wrong.

	Josh Geenen (:pi) Assignee
	Comment 19 • 17 years ago

I apologize for that problem.  It turns out that it this line is causing it:
var module = importService.GetModule("addressbook", 0);
That line gets the first module from the importService, but in Windows this isn't necessarily the module named "Text file (LDIF, .tab, .csv, .txt)" in English.

I tried the following code, which checks every module name until it finds one with .csv and it worked in Windows:

  var module;

  // get the number of address book nsIImportModules
  var count = importService.GetModuleCount("addressbook");

  //find the CSV module
  for (var i = 0; i < count; i++) 
  {
    if (importService.GetModuleName("addressbook",i).indexOf(".csv") != -1)
    {
      module = importService.GetModule("addressbook", i);
      break;
    }
  }

  // make sure the module was found
  if (!module)
    do_throw("Could not find the necessary import module");

I didn't make a patch yet until I think about it longer and can't find a better way.  Does the nsIImportModule's name change in different locales?  If so, it should still have .csv in it, right?  If the name ever doesn't contain .csv in any locale the test will fail.

WantsProgress (http://mxr.mozilla.org/seamonkey/source/mailnews/import/src/nsImportAddressBooks.cpp#475) was returning false because it was the wrong module.  If you look at line 494, count was 0 because of the different module.

	Josh Geenen (:pi) Assignee
	Comment 20 • 17 years ago

This is an updated unit test that uses a new helper script (used more in future tests).  I tested this in Gentoo Linux and Windows Vista, but not on a Mac.

	Mark Banner (:standard8)
	Comment 21 • 17 years ago

Comment on attachment 325837 [details] [diff] [review]
Updated unit test

+  var importService = Cc["@mozilla.org/import/import-service;1"]
+                        .getService(Ci.nsIImportService);

Oh what a lovely interface that is. I think you've done the best you can there. Improving it is now on my list of things to look at.

+  // Make sure the module was found.  If not, return false
+  if (!module)
+    return false;

Return null here, its clearer.

+  var errorStr =   Cc["@mozilla.org/supports-string;1"]
+                     .createInstance(Ci.nsISupportsString);

You only need one space between = and Cc.

+  do_check_false(!abInterface);

I think this would be clearer as:

do_check_neq(abInterface, null);

ditto with:
+  do_check_false(!errorStr);

r=me with those fixed.

	Josh Geenen (:pi) Assignee
	Comment 22 • 17 years ago

Fixed the mistakes.

	David :Bienvenu
	Comment 23 • 17 years ago

you only need one review for a unit test, so Standard8's review is sufficient.

	Mark Banner (:standard8)
	Comment 24 • 17 years ago

Checking in mailnews/import/test/resources/import_helper.js;
/cvsroot/mozilla/mailnews/import/test/resources/import_helper.js,v  <--  import_helper.js
initial revision: 1.1
done
Checking in mailnews/import/test/unit/head_import.js;
/cvsroot/mozilla/mailnews/import/test/unit/head_import.js,v  <--  head_import.js
new revision: 1.2; previous revision: 1.1
done
Checking in mailnews/import/test/unit/test_bug_437556.js;
/cvsroot/mozilla/mailnews/import/test/unit/test_bug_437556.js,v  <--  test_bug_437556.js
new revision: 1.3; previous revision: 1.2

	Mark Banner (:standard8)
	Comment 25 • 17 years ago

This is now working fine on the Linux & Windows unit test boxes (no Mac yet, but it works fine on my mac). Therefore marking as fixed again. Thanks Josh.