Last Comment Bug 703175 - nsImportTextAddress::DetermineDelim seems wrong
: nsImportTextAddress::DetermineDelim seems wrong
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Import (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 16.0
Assigned To: Hiroyuki Ikezoe (:hiro)
:
:
Mentors:
Depends on: 700920
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-16 19:15 PST by Hiroyuki Ikezoe (:hiro)
Modified: 2012-06-26 18:05 PDT (History)
3 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix (2.83 KB, patch)
2011-11-16 19:16 PST, Hiroyuki Ikezoe (:hiro)
mozilla: review+
Details | Diff | Splinter Review
A test (1.64 KB, patch)
2011-11-16 19:29 PST, Hiroyuki Ikezoe (:hiro)
mozilla: review+
Details | Diff | Splinter Review
De-rotted patch (2.99 KB, patch)
2012-06-25 21:44 PDT, Hiroyuki Ikezoe (:hiro)
hiikezoe: review+
Details | Diff | Splinter Review
De-rotted test (1.70 KB, patch)
2012-06-25 21:44 PDT, Hiroyuki Ikezoe (:hiro)
hiikezoe: review+
Details | Diff | Splinter Review

Description Hiroyuki Ikezoe (:hiro) 2011-11-16 19:15:16 PST
As you can see from the fact that some variable names contain 'line' word in nsImportTextAddress::DeteminDelim, The function aims to count tab and comma line by line but actually it fails.

First, NULL-terminated position is always kTextAddressBufferSz - 1, it should be read bytes from stream, in this case it's 'left' variable. (The variable name is not good at all, by the way)

Second, nsIInputStream is used instead of nsILineInputStream.
Comment 1 Hiroyuki Ikezoe (:hiro) 2011-11-16 19:16:38 PST
Created attachment 575078 [details] [diff] [review]
Fix
Comment 2 Hiroyuki Ikezoe (:hiro) 2011-11-16 19:29:12 PST
Created attachment 575079 [details] [diff] [review]
A test
Comment 3 Hiroyuki Ikezoe (:hiro) 2011-11-16 19:31:04 PST
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> Created attachment 575079 [details] [diff] [review] [diff] [details] [review]
> A test

This test depend on importHelper.js in bug 700920.
Comment 4 David :Bienvenu 2011-12-11 17:03:34 PST
Comment on attachment 575078 [details] [diff] [review]
Fix

The patch didn't apply cleanly, but when I fixed it, it passed the tests...
Comment 5 Hiroyuki Ikezoe (:hiro) 2012-06-25 21:44:15 PDT
Created attachment 636594 [details] [diff] [review]
De-rotted patch
Comment 6 Hiroyuki Ikezoe (:hiro) 2012-06-25 21:44:35 PDT
Created attachment 636595 [details] [diff] [review]
De-rotted test
Comment 7 Ludovic Hirlimann [:Usul] 2012-06-25 21:51:08 PDT
So should these be checkin-needed ?
Comment 8 Hiroyuki Ikezoe (:hiro) 2012-06-25 22:28:23 PDT
(In reply to Ludovic Hirlimann [:Usul] from comment #7)
> So should these be checkin-needed ?

Yes. Thanks for the notice. I do sometimes forget the flag.

Note You need to log in before you can comment on or make changes to this bug.