1018679 - History/Bookmarks migration from Safari fails if the corresponding Property List file uses 3-byte integers

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[Matthew N. [:MattN]]

I suspect the history format changed since the last time we tested (perhaps Safari 7?). I don't think QA is testing this stuff anymore since litmus was decommissioned as I don't see migration test cases anymore.

Browser Console Output:

Could not read history property list SafariProfileMigrator.js:202
some history did not successfully migrate.
Could not read aBuffer as a binary property list PropertyListUtils.jsm:253

Juan, could you make sure testing migration from at least the latest version of all supported browsers is part of test suites in moztrap? The earlier in the channels, the better. We have basically no automated testing for migration at this time so manual testing is needed. Thanks.

Comment 1

[Asaf Romano (gone)]

Wow, no QA for this area is done at all :(

I wonder how long is this broken.

Comment 2

[Asaf Romano (gone)]

Sigh. This is broken because the offset int size (see http://fileformats.archiveteam.org/wiki/Property_List/Binary) is 3, meaning I need to take care of int-24, as if I didn't have enough of these issue in PropertyListUtils already. :-/

Comment 3

[Bogdan Maris, Desktop QA]

I encountered this issue yesterday while testing on Firefox 31 beta 2 on Mac OS X 10.9.3. I managed to find a regression window, build from 2012-04-16 is good because there was no Safari browser in Import Wizard.

Last good revision: fd06332733e5 (2012-04-16)
First bad revision: c61e7c3a232a (2012-04-17)
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fd06332733e5&tochange=c61e7c3a232a

Let me know if there is anything I can help with here.

Comment 4

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Test cases were created, added in Moztrap, and executed using Firefox 31 Beta 2 on Windows, Mac OS and Ubuntu.

Comment 5

[Asaf Romano (gone)]

Attachment: patch

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8452291 [details] [diff] [review]
patch

Review of attachment 8452291 [details] [diff] [review]:
-----------------------------------------------------------------

While it would be nice to have a test, I assume it's hard to build a plist with a specific offset length and then in future keep it updated (we'd need sort-of an editor that keeps that special value intact).
On the other side the test may even be a specifically built plist just covering this piece of code and never be touched again.
I don't think I'm going to block on the test requirement, if we can get that it will be bonus points, but may even just be wontfix or a follow-up due to the complexity of making it generic enough. I'd be fine if in the meanwhile we'd start by landing this patch.

Comment 7

[Asaf Romano (gone)]

Yeah, the thing is, it's the "editor" (which could be xcode, plutil or some third party editor) that decides the integer size, and it seems "3" is either deprecated, or used in a very particular case, and it's not (solely) determined by the number of entries in the file (Matthew and I found out that if the binary plist is converted to the xml format (using plutil), and then back to binary format, the more common 4-byte integers are used). There are two ways to create a test here: edit Matthew's file very carefully not to loose the 3-byte format, of create the plist not through the native APIs (e.g. by using one of those perl/c# editors). Indeed, I found the value of both options quite limited. It would be more valuable if this code wasn't stable, but the way things are a test would cover typed array regressions more than anything else.

All that said, as long as I'm practically the only active developer of this module, I'll keep testing any refactoring with Matthew's file.

Comment 8

[Asaf Romano (gone)]

BTW, here's an alternative approach (see getUint24):
https://github.com/ukyo/DataViewUtils/blob/master/src/dataview.utils.js

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I don't think one approach has much advantage vs the other one, I'd be fine with both. The current one is more verbose but very explicit, while bit operators are more compact notation, maybe scarier to js contributors.
I'd probably have written the bit operators code, but just cause I wouldn't have thought of the possibility to use the buffer.
So, whatever, take the one you like more, none of them has only advantages.

Comment 10

[Asaf Romano (gone)]

https://hg.mozilla.org/integration/fx-team/rev/737cb19ee1c4

Comment 11

[Asaf Romano (gone)]

This was merged five days ago, apparently:

https://hg.mozilla.org/mozilla-central/rev/737cb19ee1c4

Comment 12

[Asaf Romano (gone)]

While still qa-, Matthew can verify this with his original test case. So I'll leave this RESOLVED for now.