nsImportTextAddress::DetermineDelim seems wrong

RESOLVED FIXED in Thunderbird 16.0

Status

MailNews Core
Import
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

Trunk
Thunderbird 16.0
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 575078 [details] [diff] [review]
Fix
Assignee: nobody → hiikezoe
Attachment #575078 - Flags: review?(dbienvenu)
(Assignee)

Comment 2

6 years ago
Created attachment 575079 [details] [diff] [review]
A test
Attachment #575079 - Flags: review?(dbienvenu)
(Assignee)

Comment 3

6 years ago
(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.
Status: NEW → ASSIGNED
Depends on: 700920

Comment 4

6 years ago
Comment on attachment 575078 [details] [diff] [review]
Fix

The patch didn't apply cleanly, but when I fixed it, it passed the tests...
Attachment #575078 - Flags: review?(dbienvenu) → review+

Updated

6 years ago
Attachment #575079 - Flags: review?(dbienvenu) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 636594 [details] [diff] [review]
De-rotted patch
Attachment #575078 - Attachment is obsolete: true
Attachment #636594 - Flags: review+
(Assignee)

Comment 6

5 years ago
Created attachment 636595 [details] [diff] [review]
De-rotted test
Attachment #575079 - Attachment is obsolete: true
Attachment #636595 - Flags: review+
So should these be checkin-needed ?
(Assignee)

Comment 8

5 years ago
(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.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/c3444a3d3436
https://hg.mozilla.org/comm-central/rev/206610bffbe7
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
You need to log in before you can comment on or make changes to this bug.