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

RESOLVED FIXED in Firefox 33

Status

()

Firefox
Migration
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: MattN, Assigned: mano)

Tracking

Trunk
Firefox 33
All
Mac OS X
Points:
5
Bug Flags:
firefox-backlog +
in-moztrap +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
Flags: in-moztrap?(jbecerra)
Flags: firefox-backlog+
Wow, no QA for this area is done at all :(

I wonder how long is this broken.
Assignee: nobody → mano
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. :-/
Summary: History cannot be migrated from Safari 7 → History cannot be migrated from Safari if History.plist uses 3-byte ints.
Summary: History cannot be migrated from Safari if History.plist uses 3-byte ints. → History/Bookmarks migration from Safari fails if the corresponding Property List file uses 3-byte integers

Updated

4 years ago
Whiteboard: p=2
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.
Test cases were created, added in Moztrap, and executed using Firefox 31 Beta 2 on Windows, Mac OS and Ubuntu.
Flags: in-moztrap?(jbecerra) → in-moztrap+

Updated

4 years ago
Points: --- → 2
Whiteboard: p=2
Points: 2 → 5

Updated

4 years ago
Status: NEW → ASSIGNED
Iteration: --- → 33.2
QA Whiteboard: [qa-]

Updated

4 years ago
Iteration: 33.2 → 33.3
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.
Attachment #8452291 - Flags: review?(mak77) → review+
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.
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.
This was merged five days ago, apparently:

https://hg.mozilla.org/mozilla-central/rev/737cb19ee1c4
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
While still qa-, Matthew can verify this with his original test case. So I'll leave this RESOLVED for now.
Target Milestone: --- → Firefox 33
You need to log in before you can comment on or make changes to this bug.