Closed Bug 397768 Opened 17 years ago Closed 17 years ago

Opera bookmark import loses last bookmark

Categories

(Firefox :: Migration, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3 beta1

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

Details

(Keywords: platform-parity)

Attachments

(2 files)

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.
Attached patch Patch rev. 1Splinter Review
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
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?
Attached file 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.
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?
Sure, filed bug 397850.
mozilla/browser/components/migration/src/nsOperaProfileMigrator.cpp 	1.72 

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

Attachment

General

Created:
Updated:
Size: