476903 - Setting nsNavHistoryQueryOptions.sortingMode to any value raises exception on Fennec

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jono Xia]

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.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 2

[Dan Mills [:thunder]]

Cc'ing Ed, he was looking at this today as well.  He did get a debug build going on scratchbox today.

Comment 3

[Ed Lee :Mardak]

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.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fix arm/gcc and CE xptcinvoke type conversions

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.

Comment 6

[Ed Lee :Mardak]

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..?

Comment 7

[Dan Mills [:thunder]]

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.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Mobile would like to get this patch landed for beta 1

Comment 9

[Benjamin Smedberg]

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))

Comment 10

[Benjamin Smedberg]

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?

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated patch with PR_STATIC_ASSERTs

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.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/6ac03fc09e48

Comment 13

[Koen Kooi]

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

Comment 14

[(not currently active) Ted Mielczarek]

PR_STATIC_ASSERT works fine everywhere else, please file a new bug on that, it's clearly something with your setup.

Comment 15

[Koen Kooi]

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.

Comment 16

[(not currently active) Ted Mielczarek]

File a new bug. This clearly works on our tinderboxes.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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).

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fixup patch

Fixup patch

Comment 19

[Ed Lee :Mardak]

Does the fixup patch need to be checked in?

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

I think vlad pushed it. We'd also need it on 1.9.1

Comment 21

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

what's the status here?

Comment 23

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

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.

Comment 24

[Samuel Sidler (old account; do not CC)]

(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.

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 26

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

commen t 18 patch has been pushed with:
http://hg.mozilla.org/mozilla-central/rev/f2ace497dea6