Closed Bug 1018679 Opened 6 years ago Closed 6 years ago
History/Bookmarks migration from Safari fails if the corresponding Property List file uses 3-byte integers
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.
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. :-/
6 years ago
Summary: History cannot be migrated from Safari 7 → History cannot be migrated from Safari if History.plist uses 3-byte ints.
6 years ago
Depends on: 738910
6 years ago
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
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+
See Also: → 1027065
Status: NEW → ASSIGNED
Iteration: --- → 33.2
QA Whiteboard: [qa-]
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.
BTW, here's an alternative approach (see getUint24): https://github.com/ukyo/DataViewUtils/blob/master/src/dataview.utils.js
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
Closed: 6 years ago
Resolution: --- → FIXED
While still qa-, Matthew can verify this with his original test case. So I'll leave this RESOLVED for now.
You need to log in before you can comment on or make changes to this bug.