Closed Bug 715074 Opened 8 years ago Closed 8 years ago
SIGBUS on unaligned access in Key::Encode
While attempting to start the gallery app on b2g, we get: Program received signal SIGBUS, Bus error. 0x8261568a in mozilla::dom::indexedDB::Key::EncodeNumber ( this=<optimized out>, aFloat=<optimized out>, aType=<optimized out>) at /fasthome/mikew/moz/B2G/gecko/dom/indexedDB/Key.cpp:414 414 /fasthome/mikew/moz/B2G/gecko/dom/indexedDB/Key.cpp: No such file or directory. in /fasthome/mikew/moz/B2G/gecko/dom/indexedDB/Key.cpp (gdb) bt #0 0x8261568a in mozilla::dom::indexedDB::Key::EncodeNumber ( this=<optimized out>, aFloat=<optimized out>, aType=<optimized out>) at /fasthome/mikew/moz/B2G/gecko/dom/indexedDB/Key.cpp:414 #1 0x82615aaa in mozilla::dom::indexedDB::Key::EncodeJSVal (this=0x4236c388, aCx=0x402c1c40, aVal=..., aTypeOffset=<optimized out>) at /fasthome/mikew/moz/B2G/gecko/dom/indexedDB/Key.cpp:155 #2 0x8260455c in SetFromJSVal (aVal=<optimized out>, this=<optimized out>, aCx=<optimized out>) at ../../dist/include/mozilla/dom/indexedDB/Key.h:166 #3 (anonymous namespace)::GetKeyFromJSVal (aCx=0x402c1c40, aVal=..., aKey=..., aAllowUnset=<optimized out>) at /fasthome/mikew/moz/B2G/gecko/dom/indexedDB/IDBKeyRange.cpp:101 #4 0x82604578 in (anonymous namespace)::GetKeyFromJSValOrThrow (aCx=0x0, aVal=..., aKey=...) at /fasthome/mikew/moz/B2G/gecko/dom/indexedDB/IDBKeyRange.cpp:132 #5 0x826047a6 in (anonymous namespace)::MakeLowerBoundKeyRange ( aCx=0x402c1c40, aArgc=<optimized out>, aVp=0x40f00110) at /fasthome/mikew/moz/B2G/gecko/dom/indexedDB/IDBKeyRange.cpp:176 #6 0x82b7ff46 in CallJSNative (args=<optimized out>, native=<optimized out>, cx=<optimized out>) at /fasthome/mikew/moz/B2G/gecko/js/src/jscntxtinlines.h:311 At that line - https://hg.mozilla.org/mozilla-central/file/a737cc816eec/dom/indexedDB/Key.cpp#l414 we see: *reinterpret_cast<PRUint64*>(buffer) = NS_SWAP64(number); "buffer" isn't aligned enough.
Jan, can you take this?
Assignee: nobody → Jan.Varga
Duplicate of this bug: 715043
mwu, could you please try this patch ?
(In reply to Jan Varga [:janv] from comment #4) > Created attachment 585802 [details] [diff] [review] > patch v0.1 > > mwu, could you please try this patch ? No crash now. The Makefile.in part didn't apply, though it doesn't matter for me. Can we add padding to align the buffer instead?
The whole point of this code is to create a compressed representation of the key as to reduce IO, so adding padding would defeat that.
In that case, I would like to see the encoding match the decoding either by using for loops or memcpy.
They can't be exactly the same since we do some post-processing after encoding. Please see last step of comment at the top of Key.cpp. But we could use memcpy on big-endian CPUs sure. But none of our tier-1 platforms are big-endian
Well, it's not exactly the same, but it's close: encode: number = NS_SWAP64(number); memcpy(buffer, &number, sizeof(number)); decode: memcpy(&number, aPos, NS_MIN(sizeof(number), aEnd - aPos)); number = NS_SWAP64(number);
And aPos would have to be incremented appropriately
(In reply to Michael Wu [:mwu] from comment #9) > Well, it's not exactly the same, but it's close: > > encode: > number = NS_SWAP64(number); > memcpy(buffer, &number, sizeof(number)); > > decode: > memcpy(&number, aPos, NS_MIN(sizeof(number), aEnd - aPos)); > number = NS_SWAP64(number); do you want this on all platforms ?
Yes, since it should work on all platforms, though we should test first to make sure that gcc picks a memcpy implementation that handles unaligned addresses. It should since we're passing in char pointers, but I'll make sure on my device.
Hmm, the current implementation should be faster, maybe twice, no ?
memcpy is generally optimized and will attempt to copy more than one byte at a time using hardware specific features if it is possible. So at the very least, decoding might be faster because we only increment aPos once and memcpy may copy more than one byte at a time. In the encoding case, the copy is always a fixed size, so the compiler may actually just replace memcpy with some simple copy instruction(s) on platforms where unaligned writes are supported. But I really just want to do memcpy all the time to keep the code simple.
ok, let's use memcpy
Actually, given that we're copying at the most 8 bytes here, I don't think there's any benefit to copying multiple bytes at a time. An unrolled loop is likely the fastest implementation. However given that performance of the actual mem copy here is very unlikely to matter, I think we should use memcpy everywhere since it's the smallest amount of code, which is nice. Note that it's fine to always do aPos += 8. It's ok for aPos to point past aEnd. We end up in that situation elsewhere already.
NS_MIN<size_t>(a, b) should work as well
(In reply to Ms2ger from comment #17) > NS_MIN<size_t>(a, b) should work as well ok, thanks
We're running IDB tests on fennec right? Did we just happen to hit a funny case on b2g? If so, can we get a test for that here?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #20) > Did we just happen to hit a funny case on b2g? s/b2g/gaia/
The basic indexedDB tests should be testing this quite extensively. Do we have those tests disabled on fennec by any chance?
Attachment #586063 - Flags: review?(jonas) → review+
This should be landed on FF11 too.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #20) > We're running IDB tests on fennec right? Did we just happen to hit a funny > case on b2g? If so, can we get a test for that here? I don't think we're running these tests on fennec.
landed on the trunk https://hg.mozilla.org/mozilla-central/rev/8ae16e346bd0
Please nominate for Beta 11 approval ASAP if we'd like to get land prior to release, the user effect is significant, and the patch is considered low risk.
Attachment #586063 - Flags: approval-mozilla-beta?
Ben - can you fill out the form that is pre-populated in the comment when requesting beta approval? We'd like to better understand the risk and user benefit.
Comment on attachment 586063 [details] [diff] [review] patch v0.2 Sorry, I misinterpreted your earlier comment (thought that you knew all this already since you were suggesting we nominate): [Approval Request Comment] Regression caused by (bug #): 692614 User impact if declined: IndexedDB crashes almost instantly on ARM devices, makes it impossible to use. Testing completed (on m-c, etc.): Lots of m-c testing. Risk to taking this patch (and alternatives if risky): Really low risk, we're using this code on desktop all the time and haven't had any problems. String changes made by this patch: None
Comment on attachment 586063 [details] [diff] [review] patch v0.2 [Triage Comment] Given the low risk nature of this patch, and where we are in the cycle, approving for Beta.
Attachment #586063 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.