Closed Bug 1333518 Opened 3 years ago Closed 3 years ago

Bookmarks import imports Samsung Internet folders as bookmarks with null url


(Firefox for Android :: Data Providers, defect)

Not set



Firefox 54
Tracking Status
firefox54 --- fixed


(Reporter: twointofive, Assigned: twointofive)



(1 file)

When we import bookmarks from Samsung Internet, the Samsung Internet content provider actually provides both bookmarks and folders, distinguished by "bookmark_type" (equals 0 for folder, 1 for bookmark - there's no heirarchical data, just "is this item a bookmark or a folder").  We're currently importing both types, with folders getting inserted into our db with a url of null.  Such "bookmarks" never seem to surface in UI, and sync (seemingly) ignores them, but it would be nice to stop importing them anyway.

I've tested on my one device, which is at 5.0.1/api 21.  Importing is turned off for 23+, so we only need to worry about what happens for 15-22.  My only concern is that possibly the "bookmark_type" column doesn't exist all the way back to 15 (we would crash with this patch if it doesn't exist).  I can't find any documentation on the web for content:// (maybe it's no longer supported?  I would think maybe it would be somewhere around here if it was: but I don't find anything), so I think the only way to find out is to get a device at api 15 with Samsung Internet and try it out.  I don't have such a device, and I can't find any apks for Internet supporting 15 to try to install on an emulator (plus all of my attempts to install Internet on later API emulators have failed as well :/).  Any thoughts or suggestions here?  Thanks!
By the way we always import at least Internet's base bookmark folder "'My phone" even if the user hasn't created any other bookmark folders.
Oh yeah, and I guess this needs to be tested on API 22 as well since I don't actually know if/when the Internet content provider was disabled.  As I mentioned, my attempts to install a Samsung Internet apk on an emulator have all failed.  For one I only find arm apks, and arm emulators are extremely slow (to the point of erroring out on timeouts), and each time I've attempted to install I get an error like the following:

klein:tmp$ adb install
[100%] /data/local/tmp/
        pkg: /data/local/tmp/

I can't find any indication of which library is missing, but I guess Internet is only supported on certain Samsung devices (?) so maybe it requires some library bundled with those devices.
Ugh, sorry, got ahead of myself in comment 3 - presumably the Internet bookmarks content provider is still functional in api 22 or we would have gotten reports about it.  I suppose it's possible they removed the "bookmark_type" field in 22, but that seems unlikely.
I don't think this is a very important bug for the time I'm spending on it, but I'm learning something (I think), so that's good :)  (Yes it's slightly embarassing to be learning basic things in front of the entire world - and apologies for the extra comments it's generating.  Hopefully they're not all silly this time.)

So I guess ideally the way the columns provided by an app's content provider are specified (when they're specified at all) is via a public contract class for the content provider.  Only as far as I can tell in the examples I've seen between apps, that's not really done programatically, as in the answer here: (which seems like it would be yuck anyway to check programatically if a bunch of columns you want to use exist). Instead we just crash (or catch) and then deal with it if a column doesn't exist for whatever reason for the version of the app we happen to be querying?  I would think that should only be an issue for columns that got added to a contract at some point (ideally a contract in one version of the app remains valid in all future versions of the app): in that case a column is available on new versions of the app but will cause exceptions when queried with older versions of the app.  That's my concern here since I don't know when Internet started providing a "bookmark_type" column.  (The worrying I was doing in earlier comments about columns disappearing based on API version was probably silly (though not inconceivable).)

In this case there seems to be no online documentation for Internet's bookmarks content provider, and the source code (for only v3 and v4 of the app) available here
downloads (for me) at 30Kb/sec, is 1.2G of code, and each time the download fails you need to start from scratch because you need to accept some agreement, so is not really available, at least to me.

So unless anybody has other advice, I think I've done all I can, which is solely to verify that touching the column "bookmark_type" is safe for whatever version of Internet I have on my phone, on my particular phone. :P  That's a rather bleak assessment, and I don't think the actual situation is as bad as I'm putting it, but I'll just say I won't be offended if it's decided the risk in this case isn't worth the gain.  Of course a reasonable fix here I think is to just tighten down the patch and catch if "bookmark_type" doesn't exist for whatever reason, but then the import fails where maybe it didn't before, which wouldn't be very nice (I think there's actually already a bug somewhere for not giving a message when import fails).

Okay, I think I'm done talking now.
Comment on attachment 8829975 [details]
Bug 1333518 - Don't import Samsung Internet folders as bookmarks.

In theory our code and UI has no problem handling folders too. But it seems like we only intend to import actual bookmarks (hardcoded type in INSERT) - so r+.
Attachment #8829975 - Flags: review?(s.kaspari) → review+
Pushed by
Don't import Samsung Internet folders as bookmarks. r=sebastian
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
You need to log in before you can comment on or make changes to this bug.