Setting nsNavHistoryQueryOptions.sortingMode to any value raises exception on Fennec

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: jono, Assigned: vlad)

Tracking

Trunk
ARM
Maemo
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec1.0b1+)

Details

Attachments

(3 attachments)

Reporter

Description

11 years ago
Here's my code:
 let options = this._hsvc.getNewQueryOptions();
 options.sortingMode = options.SORT_BY_DATE_DESCENDING;

On Firefox, it works fine.  On the Mac build of Fennec 1.0a2 it works fine.  On Fennec on the Nokia, the second line raises NS_ERROR_INVALID_ARG.

The code at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistoryQuery.cpp#1164
indicates that the only way NS_ERROR_INVALID_ARG should be raised is if the argument passed in is greater than SORT_BY_ANNOTATION_DESCENDING.  Dumping the values verifies that SORT_BY_ANNOTATION_DESCENDING is 20 and SORT_BY_DATE_DESCENDING is 4, so it looks like Places thinks 4 > 20 on Fennec.

I wrote a loop with a try/catch to see exactly what values cause the exception to be raised.  Turns out that ANY value from -1 to 20 causes the same exception.

Updated

11 years ago
Blocks: 468683
SORT_BY_DATE_DESCENDING _does_ exist in Maemo builds. Running the following from xpcshell prints out all the expected CONST values:

for (prop in Components.interfaces.nsINavHistoryQueryOption) print(prop)

I haven't be able to get Fennec running in scratchbox, so I'll need to debug it on the device.

I notice that the SORT_BY_* constants are unsigned short, while others are unsigned long. I'm pretty sure Fennec is using some of the unsigned long constants. More digging needed.
Cc'ing Ed, he was looking at this today as well.  He did get a debug build going on scratchbox today.

Comment 3

11 years ago
Right now it seems like aMode's value as a short is the correct value passed in, but the compiler is under the assumption that the value is sign extended to 4 bytes.

So the emitted machine code just does a plain "cmp r1, #20", but there's junk in the top 2 bytes causing the comparison to be the wrong way.
Arm XPTCall stuff is broken -- we are marshalling function arguments incorrectly.  Any integral types smaller than 4 bytes need to be sign/zero-extended to 4 bytes before being placed in registers/on the stack; we weren't doing that.  Patch coming up.
Assignee: nobody → vladimir
Component: General → XPCOM
Product: Fennec → Core
QA Contact: general → xpcom
This should fix it; the only thing I'm a little shaky about is the signedness of char/wchar_t.  This was based on compiling a test program and looking at the assembly with msvc and gcc, but on gcc at least, -fsigned-wchar/-fsigned-char etc. can modify this.  No idea if we can test for this at compile time usefully.. worst case I can probably add an if() that the compiler will optimize away statically at compile time.
Attachment #363974 - Flags: review?(benjamin)
tracking-fennec: --- → ?

Comment 6

11 years ago
This bug prevents us from doing Weave history sync on Fennec, and that was an item for Weave milestone 4.

thunder: Do we want to try getting this fix in before M4? I suppose it wouldn't really help Fennec users unless they're running trunk anyway, so maybe not..?
I'm okay with it slipping past Weave M4 (tomorrow), but would really like for this to go into Fennec 1.0b1 (this week? next week?).  Otherwise history sync on Fennec will remain untested.
Mobile would like to get this patch landed for beta 1

Updated

11 years ago
tracking-fennec: ? → 1.0b1+

Updated

11 years ago
Attachment #363974 - Flags: review?(benjamin) → review-

Comment 9

11 years ago
Comment on attachment 363974 [details] [diff] [review]
fix arm/gcc and CE xptcinvoke type conversions

>diff --git a/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp b/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp
>--- a/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp
>+++ b/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp
>@@ -126,24 +126,28 @@
>             *((void**)d) = s->ptr;
>             continue;
>         }
>+        // According to the ARM EABI, integral types that are smaller than a word
>+        // are to be sign/zero-extended to a full word and treated as 4-byte values.
>+        // NOTE: on ARM/gcc, both char and wchar_t are unsigned.  This assumes
>+        // no -fsigned-wchar; is there a #define we can check for that?

You might be able to use a static assert (you'll have to create a little testcase):

PR_STATIC_ASSERT(wchar_t(0xFFFF) == PRUint32(0xFFFF))

Updated

11 years ago
Attachment #363974 - Flags: review- → review+

Comment 10

11 years ago
Comment on attachment 363974 [details] [diff] [review]
fix arm/gcc and CE xptcinvoke type conversions

argh, this was supposed to be r+

Is this really not covered by unit tests? Or are we just not running these tests on ARM devices yet?
PR_STATIC_ASSERT seems to work great; I also switched wchar_t to PRUnichar, since PRUnichar is the actual type that's in the XPCVariant.
http://hg.mozilla.org/mozilla-central/rev/6ac03fc09e48
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED

Comment 13

10 years ago
This isn't fixed, the PR_STATIC_ASSERT code is broken:

11:49 < mru> Pavlov: wtf is that supposed to do?
11:50 < mru> ah, I see
11:50 < Pavlov> compile time fail
11:50 < Pavlov> if not true
11:50 < mru> it's supposed to be an error
11:50 < mru> but that's not valid c
11:50 < mru> it relies on the compiler optimising the constant expression
11:53 < mru> extern char pr_static_assert[1 - !(condition)] should work
11:55 < mru> or maybe the condition is failing
11:55 < mru> koen: does it work -fno-signed-char ?

The problem is that for some reason the mozilla buildsystem adds -fsigned-char to my build, resulting ing:

/OE/angstrom-dev/work/armv7a-angstrom-linux-gnueabi/fennec-1_0.9+1.0b2pre-r3/mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp:134: error: size of array 'arg' is negative

My fix:

       for i in $(find ${S} -name "autoconf.mk") ; do
               sed -i -e s:fsigned-char:fno-signed-char:g $i
       done
PR_STATIC_ASSERT works fine everywhere else, please file a new bug on that, it's clearly something with your setup.

Comment 15

10 years ago
the PR_STATIC_ASSERT relies on -fno-signed-char while the *mozilla* buildsystem adds -fsigned-char. I have *no mention at all* of -f(no-)signed-char in my buildscripts, so this is a mozilla problem. So either fix the asserts to work with both no-signed-char and signed-char or stop the buildsystem from including -fsigned-char.
File a new bug. This clearly works on our tinderboxes.
No, the patch wasn't quite right.. I wasn't thinking straight.  Casting to PRInt32 is the right thing to do in all cases, since that will guarantee sign extension if necessary (signed char/wchar types) or no extension (unsigned types).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #368529 - Flags: review? → review?(benjamin)

Updated

10 years ago
Attachment #368529 - Flags: review?(benjamin) → review+

Comment 19

10 years ago
Does the fixup patch need to be checked in?
I think vlad pushed it. We'd also need it on 1.9.1
Blocks: 497359
what's the status here?
No longer blocks: 497359
Duplicate of this bug: 497359
asking wanted, this issue makes some parts of Places not working at all, would be cool to fix it in a maintenance 1.9.1.x release, mostly for new platforms based on ARM processors.
Flags: wanted1.9.1.x?
(In reply to comment #23)
> asking wanted, this issue makes some parts of Places not working at all, would
> be cool to fix it in a maintenance 1.9.1.x release, mostly for new platforms
> based on ARM processors.

Those products are now going to be based on 1.9.2. Not wanted, but feel free to renominate if I'm wrong.
Flags: wanted1.9.1.x?
Fennec is building from 1.9.2, which appears to have this patch in it. I think we can close this bug.

Reopen if needed.
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
commen t 18 patch has been pushed with:
http://hg.mozilla.org/mozilla-central/rev/f2ace497dea6
You need to log in before you can comment on or make changes to this bug.