265503 - on camino branch, importing firefox live bookmarks causes an incorrect import

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Geoff "temporarily off bugmail" Beier]

garbage (like "tr>") gets entered for some bookmarks' keywords and description
if any "live bookmarks" have been created in firefox.

Comment 1

[Geoff "temporarily off bugmail" Beier]

This was originally tested using 0.8.1 and "latest-0.8" from ftp.mozilla.org.
Having looked at this with my own branch build, built from the latest sources on
the branch as of 10/31, this problem no longer appears to occur. Could someone
confirm, and I'll close?

Comment 2

[Mike Pinkerton (not reading bugmail)]

what did we change that would have caused this to magically fix itself? i'd like
to know a little better before we just close it.

Comment 3

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #2)
> what did we change that would have caused this to magically fix itself? i'd like
> to know a little better before we just close it.

according to your comment on bug 243510 it landed on the branch. Looking at the
binary from ftp.mozilla.org, it'd have been created before that patch landed; my
money's on that patch fixing it. The original reason I was investigating ths was
that kerz reported a UI hang. I still have not managed to duplicate that either
with my own builds or ftp.m.o builds.

If possible, I'd like someone else to confirm that this works for them before I
close.

Comment 4

[Jasper]

Ok I will back this up, I tested with both a recent trunk build aswell with
0.8.2 release candidate and all works like it should.

WFM

Comment 5

[Mike Pinkerton (not reading bugmail)]

kerz tried this and it failed for him again. his ff bookmarks were at:

/Users/kerz/Library/Application support/firefox/profiles/asdfasdf​
a.slt/bookmarks.html

and they didn't show up in the menu. when he chose them manually, they froze up
the UI.

Comment 6

[Mike Pinkerton (not reading bugmail)]

here's the crash stack i get when importing kerz's bookmarks in 0.8.1

Thread 0:
0   <<00000000>> 	0xffff8cac __memcpy + 0x50c
1   com.apple.CoreFoundation       	0x90195360 __CFStringCreateImmutableFunnel3
+ 0x75c
2   com.apple.CoreFoundation       	0x901a74dc CFStringCreateWithSubstring + 0x250
3   com.apple.Foundation           	0x909fdb60 -[NSCFString substringWithRange:]
+ 0xac
4   com.apple.Foundation           	0x90a0bea4 -[NSString substringFromIndex:] +
0x5c
5   org.mozilla.navigator          	0x0005875c -[BookmarkManager
readHTMLFile:intoFolder:] + 0x4c8
6   org.mozilla.navigator          	0x00057e84 -[BookmarkManager
importBookmarks:intoFolder:] + 0xec
7   org.mozilla.navigator          	0x000551a0 -[BookmarkImportDlgController
beginImportFrom:intoFolder:] + 0x70
8   com.apple.AppKit               	0x92e773ac -[NSApplication
sendAction:to:from:] + 0x6c

Comment 7

[Mike Pinkerton (not reading bugmail)]

crashes 082RC also but no stack given by the OS.

Comment 8

[Mike Pinkerton (not reading bugmail)]

the real path from kerz is:

/Users/kerz/Library/Application Support/Firefox/Profiles/f3xrz3dk​
.default/bookmarks.html

Comment 9

[Mike Pinkerton (not reading bugmail)]

i don't know where the "​" keeps coming from, whether it's actually in his
path or it's a side-effect of copying from colloquy.

Comment 10

[Mike Pinkerton (not reading bugmail)]

Attachment: fix 2 problems in kerz's bookmarks

kerz's bookmarks had two separate problems:

1) a bookmark of the form

<a href="..." attrib1="" attrib2=""</a>

which is totally invalid html, but caused us to crash. This has been fixed, we
just skip giving it a title

2) a bookmark with a JS shortcut url w/out a trailing quote and some other
garbage HTML tossed in for good measure:

<a href="..." shortcuturl="javascript:........<B><IMG src=...">title</a>

good times!

Comment 11

[Mike Pinkerton (not reading bugmail)]

what this little exercise has taught me is that we're pretty well fucked for
most of our bookmark import code. It's very fragile and doesn't stand up well to
breaking the assumptions about well formed inputs. Either we can keep our
fingers crossed, or rewrite the whole shebang.

regardless, we should take this patch for trunk and 082, it'll help a bunch.

Comment 12

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #11)
> what this little exercise has taught me is that we're pretty well fucked for
> most of our bookmark import code. It's very fragile and doesn't stand up well to
> breaking the assumptions about well formed inputs. Either we can keep our
> fingers crossed, or rewrite the whole shebang.
> 
> regardless, we should take this patch for trunk and 082, it'll help a bunch.

This patch fixes the crashes but still does not properly import all of the
bookmarks in a particularly "good" bookmarks file. It also does not, AFAICT,
worsen any bookmark imports that worked before, so it's clearly a net
improvement. Is it worth trying to fix the correctness of the import at this
point, or should we simply shoot for fixing the crashes here?

Comment 13

[Josh Aas]

Geoff - do you intent to r- based on your comment? I don't think rewriting our
importing right now is necessary, though it may be nice. In any case, concerns
about whether or not to rewrite should not hold up this patch.

Comment 14

[Josh Aas]

Comment on attachment 166103 [details] [diff] [review]
fix 2 problems in kerz's bookmarks

looks good and works for me

Comment 15

[Geoff "temporarily off bugmail" Beier]

Comment on attachment 166103 [details] [diff] [review]
fix 2 problems in kerz's bookmarks

Based on our conversation in IRC, it seems clear that this is the best we can
reasonably do with this parser. So r=[e-mail].

Comment 16

[Mike Pinkerton (not reading bugmail)]

josh: can you land this on the branch and trunk for me? i'm at home today w/out
a branch or up to date trunk tree.

Comment 17

[Josh Aas]

landed on trunk and branch