775032 - System Message Handler - Fix B2G Installation Bug (MOZ_SYS_MSG not Defined Correctly)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Gene Lian (I already quit Mozilla)]

MOZ_SYS_MSG is not defined in B2G, thus disabling the System Message functions.

Comment 1

[Gene Lian (I already quit Mozilla)]

Attachment: WIP Patch

Fabrice,

After some experiments, I eventually found out the |MOZ_SYS_MSG| is not well defined in Navigator.cpp, thus making the following error:

JS> navigator.mozSetMessageHandler("alarm", function(e){});
Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIDOMNavigatorSystemMessages.mozSetMessageHandler]

Although I'm not an expert of Makefile, I tried to add |AC_DEFINE(MOZ_SYS_MSG)| in the configure.in and everything works fine now ;) (i.e. the SystemMessageManager is correctly initialized and the app can successfully receive the system message from parent process). However, I'm not sure if this change would turn on other non-b2g logics. Need your feedback.

Also, it seems the uuid in the IDL should match the ones in JS and manifest files since lots of existing codes are following this rule (ex, contact, settings... etc). Note that this change is not the root cause of this bug.

Does this change seem reasonable to you?

Thanks,
Gene

Comment 2

[[:fabrice] Fabrice Desré]

(In reply to Gene Lian [:gene] from comment #1)
> Created attachment 643309 [details] [diff] [review]
> WIP Patch
> 
> Fabrice,

> Although I'm not an expert of Makefile, I tried to add
> |AC_DEFINE(MOZ_SYS_MSG)| in the configure.in and everything works fine now
> ;) (i.e. the SystemMessageManager is correctly initialized and the app can
> successfully receive the system message from parent process). However, I'm
> not sure if this change would turn on other non-b2g logics. Need your
> feedback.

I'm not sure why we would need that. Can you ask review to a build peer? (eg. khuey)

> Also, it seems the uuid in the IDL should match the ones in JS and manifest
> files since lots of existing codes are following this rule (ex, contact,
> settings... etc). Note that this change is not the root cause of this bug.
> 
> Does this change seem reasonable to you?

No, these changes are not needed. There's no reason to use the same uuid for the interfaces and and the components contract ids.

Comment 3

[Gene Lian (I already quit Mozilla)]

Attachment: Patch

Hi Kyle,

We encountered some run-time error that the System Message API doesn't work for the B2G platform. The |MOZ_SYS_MSG| is not defined in the |Navigator.cpp|, thus disabling the System Message methods (please see http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp).

After some experiments, I found out if we recover the configure option originally designed by Fabrice (please see https://bugzilla.mozilla.org/show_bug.cgi?id=755245#c65), everything works well! Another alternative is simply adding |AC_DEFINE(MOZ_SYS_MSG)| before |AC_SUBST(MOZ_SYS_MSG)|, which also works.

Could you please have a review on this patch? Honestly, I'm not a real expert of Makefile and building process. Please give us some suggestions if this change doesn't seem reasonable to you. Thank you!

Fabrice,

I also had a tiny change on the function return type of |Navigator::EnsureMessagesManager()|, which should match the prototype in the |Navigator.h|. Right? I noticed this because it's not compilable for Win opt/debug builds (please see the try-server result at https://tbpl.mozilla.org/?tree=Try&rev=f01e16902742). The build error would show up after adding |AC_DEFINE(MOZ_SYS_MSG)| to enable |MOZ_SYS_MSG|.

Thanks,
Gene

Comment 4

[[:fabrice] Fabrice Desré]

Comment on attachment 643732 [details] [diff] [review]
Patch

Review of attachment 643732 [details] [diff] [review]:
-----------------------------------------------------------------

I really don't understand why you need this AC_DEFINE. We set MOZ_SYS_MSG in b2g/confvars.sh, and use it successfully for Web Activities without further modifications.

::: configure.in
@@ +7408,5 @@
> +    MOZ_SYS_MSG=1,
> +    MOZ_SYS_MSG= )
> +if test -n "$MOZ_SYS_MSG"; then
> +    AC_DEFINE(MOZ_SYS_MSG)
> +fi

We don't want that. khury already r- this earlier ;)

Comment 5

[Gene Lian (I already quit Mozilla)]

(In reply to Fabrice Desré [:fabrice] from comment #4)
> Comment on attachment 643732 [details] [diff] [review]
> Patch
> 
> Review of attachment 643732 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I really don't understand why you need this AC_DEFINE. We set MOZ_SYS_MSG in
> b2g/confvars.sh, and use it successfully for Web Activities without further
> modifications.

That's very strange... It doesn't work on our end (we used the normal building commands like |./build.sh| or |./build.sh gecko|). However, the |MOZ_SYS_MSG| is not defined in the |Navigator.cpp|, thus disabling the system message methods. 

Could you please try it again by adding some non-compile codes in both #ifdef MOZ_SYS_MSG and #else? You would probably find out the B2G build would go into the #else logic (i.e. return NS_ERROR_NOT_IMPLEMENTED). Btw, we're building B2G for Galaxy-S2-ICS.

Comment 6

[Gene Lian (I already quit Mozilla)]

(In reply to Fabrice Desré [:fabrice] from comment #4)
> Comment on attachment 643732 [details] [diff] [review]
> Patch
> 
> Review of attachment 643732 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I really don't understand why you need this AC_DEFINE. We set MOZ_SYS_MSG in
> b2g/confvars.sh, and use it successfully for Web Activities without further
> modifications.

I got it! Please see: https://developer.mozilla.org/en/configure.in, it seems the AC_DEFINE is necessary if we want to use #ifdef in C/C++ code more than Makefile. I'll upload a new patch later (no need the option part).

Gene

Comment 7

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V2

Hi Fabrice and Kyle,

The following summarizes the changes for fixing this issue:

1. We need to add |AC_DEFINE(MOZ_SYS_MSG)| if we want to use #ifdef in C/C++ code more than Makefile; otherwise, the System Message APIs would be disabled in the B2G build. Please see https://developer.mozilla.org/en/configure.in

2. Add an |MOZ_SYS_MSG=| as the default value. Only the B2G build can turn it on by running the |b2g/confvars.sh| with |MOZ_SYS_MSG=1|. Note that the default |MOZ_SYS_MSG=| won't go into the |if test -n "$MOZ_SYS_MSG"; then| block.

3. Some minor code clean-ups for alignment.

4. Correct the return type of |Navigator::EnsureMessagesManager()|, which should match the prototype in |Navigator.h|; otherwise, it won't be compilable for Win opt/debug builds (please see the try-server result at https://tbpl.mozilla.org/?tree=Try&rev=f01e16902742).

Thanks for your reviews. Please feel free to let me know if I can do anything better. :)

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 644849 [details] [diff] [review]
Patch, V2

Review of attachment 644849 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing Fabrice's review.
Can you copy what has been done for RIL and Bluetooth so it will be possible to make it works for desktop build too with a --enable-system-message flag in mozconfig.

Comment 9

[[:fabrice] Fabrice Desré]

(In reply to Vivien Nicolas (:vingtetun) from comment #8)
> Comment on attachment 644849 [details] [diff] [review]
> Patch, V2
> 
> Review of attachment 644849 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Stealing Fabrice's review.
> Can you copy what has been done for RIL and Bluetooth so it will be possible
> to make it works for desktop build too with a --enable-system-message flag
> in mozconfig.

I don't think we want to expose this like that.

And it doesn't matter much, but I still don't get why activities are working without this patch but not the alarm API...

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Fabrice Desré [:fabrice] from comment #9)
> (In reply to Vivien Nicolas (:vingtetun) from comment #8)
> > Comment on attachment 644849 [details] [diff] [review]
> > Patch, V2
> > 
> > Review of attachment 644849 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Stealing Fabrice's review.
> > Can you copy what has been done for RIL and Bluetooth so it will be possible
> > to make it works for desktop build too with a --enable-system-message flag
> > in mozconfig.
> 
> I don't think we want to expose this like that.

Whatever the solution is we need a way to enable that on a desktop build. If you have anything else to propose I'm fine with it.

> 
> And it doesn't matter much, but I still don't get why activities are working
> without this patch but not the alarm API...

Activities does not work without the MOZ_SYS_MSG flag. In fact I was opening a new bug with a patch when I fall on this one.

Comment 11

[[:fabrice] Fabrice Desré]

Vivien, MOZ_SYS_MSG is set in b2g/confvars.sh

Comment 12

[Gene Lian (I already quit Mozilla)]

(In reply to Fabrice Desré [:fabrice] from comment #9)
> (In reply to Vivien Nicolas (:vingtetun) from comment #8)
> > Comment on attachment 644849 [details] [diff] [review]
> > Patch, V2
> > 
> > Review of attachment 644849 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Stealing Fabrice's review.
> > Can you copy what has been done for RIL and Bluetooth so it will be possible
> > to make it works for desktop build too with a --enable-system-message flag
> > in mozconfig.
> 
> I don't think we want to expose this like that.
> 
> And it doesn't matter much, but I still don't get why activities are working
> without this patch but not the alarm API...

I'm wondering this as well (that's really magical...;)). Just some quick quessing if you're using deprecated codes? Or define the |MOZ_SYS_MSG| somewhere in addition to configure.in and b2g/confvars.sh? Or the Web activities only use the SystemMessageInternal part instead of the SystemMessageManager part?

I'm pretty sure it's a real issue on our end and should have nothing to do with Alarm API, because I've tested my colleagues' B2G phones and neither of them can successfully call |navigator.mozSetMessageHandler("foo", function(mesg){});| which would go into the NS_ERROR_NOT_IMPLEMENTED logic.

Comment 13

[Gene Lian (I already quit Mozilla)]

(In reply to Vivien Nicolas (:vingtetun) from comment #10)
> (In reply to Fabrice Desré [:fabrice] from comment #9)
> > (In reply to Vivien Nicolas (:vingtetun) from comment #8)
> > > Comment on attachment 644849 [details] [diff] [review]
> > > Patch, V2
> > > 
> > > Review of attachment 644849 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Stealing Fabrice's review.
> > > Can you copy what has been done for RIL and Bluetooth so it will be possible
> > > to make it works for desktop build too with a --enable-system-message flag
> > > in mozconfig.
> > 
> > I don't think we want to expose this like that.
> 
> Whatever the solution is we need a way to enable that on a desktop build. If
> you have anything else to propose I'm fine with it.
> 
> > 
> > And it doesn't matter much, but I still don't get why activities are working
> > without this patch but not the alarm API...
> 
> Activities does not work without the MOZ_SYS_MSG flag. In fact I was opening
> a new bug with a patch when I fall on this one.

Vivien,

Just like Fabrice's original design, he's directly using the b2g/confvars.sh to set the |MOZ_SYS_MSG| flag instead of using the |--enable-system-message| option (actually, this option has been reviewed- by Kyle at https://bugzilla.mozilla.org/show_bug.cgi?id=755245#c64). I think the root cause is still because the lack of AC_DEFINE().

May I have your review+ to have the check-in? Please let me know if anything doesn't sound reasonable to you. Thank you! :)

Comment 14

[Gene Lian (I already quit Mozilla)]

Comment on attachment 644849 [details] [diff] [review]
Patch, V2

Please see the previous comment. Thanks!

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 644849 [details] [diff] [review]
Patch, V2

Review of attachment 644849 [details] [diff] [review]:
-----------------------------------------------------------------

I guess you're right about AC_DEFINE. Thanks.

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f08e8bc175f2

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f08e8bc175f2