Opera bookmark import loses last bookmark

RESOLVED FIXED in Firefox 3 beta1

Status

()

Firefox
Migration
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

({pp})

unspecified
Firefox 3 beta1
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

11 years ago
Opera bookmark import loses last bookmark.

STEPS TO REPRODUCE
1. start Opera 9.22 and create some bookmarks
2. exit Opera
3. start Firefox with a clean profile
4. File->Import, select "Opera", click Next, Next, Finish

ACTUAL RESULTS
All bookmarks are imported, except the last one.

EXPECTED RESULTS
All bookmarks are imported.

PLATFORMS AND BUILDS TESTED
Bug occurs in Firefox trunk 2007092604 on Linux.
Bug occurs in Firefox 2.0.0.7 on Linux.
(Assignee)

Comment 1

11 years ago
Created attachment 282554 [details] [diff] [review]
Patch rev. 1

This fixes it for me on Linux. Not tested on other platforms.
Attachment #282554 - Flags: review?(mconnor)
Assignee: mats.palmgren → nobody
Component: Bookmarks → Migration
QA Contact: bookmarks → migration

Updated

11 years ago
Attachment #282554 - Flags: review?(mconnor)
Attachment #282554 - Flags: review+
Attachment #282554 - Flags: approval1.9+
I can test this on Windows, and land it on the trunk.

for "// XXX Todo: |description| is not saved.", I've logged bug #397771
Assignee: nobody → mats.palmgren
Target Milestone: --- → Firefox 3 M9
I have not yet able to reproduce the original problem on windows, but I can still test the patch.

note, since we check if name or url are non-empty, this patch should not hurt windows (if we make another pass once moreData is false).
Status: NEW → ASSIGNED
mats, can you elaborate on what bookmarks you are creating in opera and where?

I'm using Opera on 9.23.

Can you attach your opera6.adr file to this bug, or email it to me?
(Assignee)

Comment 5

11 years ago
Created attachment 282637 [details]
opera6.adr (Linux)

(In reply to comment #4)
> can you elaborate on what bookmarks you are creating in opera and where?

Using an empty home directory:
1. start Opera 9.22 on Linux (click through licenses etc...)
2. visit http://www.dn.se/
3. Bookmarks->Bookmark page...  click OK in the dialog that opens
4. visit http://svt.se/
5. Bookmarks->Bookmark page...  click OK in the dialog that opens
6. quit

The resulting opera6.adr is attached.

I tried the same on Windows and compared the opera6.adr files and the
only difference I can see is the different line endings, the file
ends in \r\n\r\n on Windows vs. \n\n on Linux.
(Assignee)

Comment 6

11 years ago
The reason for the different behaviour is deep down in NS_ReadLine.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/base/public/nsReadLine.h&rev=1.8&root=/cvsroot&mark=160,168,169-170,195-196#136

For the last line ending on Windows (\r\n) we will enter the block at
line 169, Append an empty string, iterate again and return on line 168 -
|*more| will be true in this case, causing an extra call by consumers
that loop it's false.
This is different from Linux where the last line ending (\n) also enter
the block at line 169, Append an empty string, but then exit the loop
since we ran out of buffer data, causing a new Read on line 191 (to check
for \n\r), it's not, so we return on line 196 with |*more| == false.

Thinking about this for a while I don't really see an error with the
above, but I guess we could lie (remove line 195) to make it behave
the same regardless of which line ending is in use.
(alternatively, peek 1 char if we find \r\n at end-of-buffer)

BTW, line 167 is pointless, AFAICT.
Keywords: pp
> BTW, line 167 is pointless, AFAICT.

I think it's an early carryover of an early patch iteration when eolStarted was a member...

As for the rest, we could change this to be consistent, sure.  But the consumer here is obviously incorrect before this patch: nsILineInputStream clearly documents that a false return still means you have to process aLine if you want to read all the data.
On second thought, I do think we should fix the \r\n handling to not effectively pretend that there is another blank line.  Mats, you want to do it?
(Assignee)

Comment 9

11 years ago
Sure, filed bug 397850.
(Assignee)

Comment 10

11 years ago
mozilla/browser/components/migration/src/nsOperaProfileMigrator.cpp 	1.72 

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.