1005093 - Notification System message not sent as expected

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Runtime, defect)

Description

[:gerard-majax]

While rebasing bug 967475 after bug 963234 landed, I noticed that the function  getNotificationURLFor() in b2g/components/AlertsHelper.jsm sometimes returns undefined value.

The code is the following:
>         let getNotificationURLFor = function(messages) {
>           if (!messages) {
>             return null;
>           }
>       
>           for (let i = 0; i < messages.length; i++) {
>             let message = messages[i];
>             if (message === kNotificationSystemMessageName) {
>               return helper.fullLaunchPath();
>             } else if (typeof message === "object" &&
>                        kNotificationSystemMessageName in message) {
>               return
>                 helper.resolveFromOrigin(message[kNotificationSystemMessageName]);
>             }
>           }
>   
>           // No message found...
>           return null;
>         }   

However, adding an intermediate variable when doing the helper.resolveFromOrigin() call makes the problem go totally away:

>         let getNotificationURLFor = function(messages) {
>           if (!messages) {
>             return null;
>           }
>       
>           for (let i = 0; i < messages.length; i++) {
>             let message = messages[i];
>             if (message === kNotificationSystemMessageName) {
>               return helper.fullLaunchPath();
>             } else if (typeof message === "object" &&
>                        kNotificationSystemMessageName in message) {
>               let retval =   helper.resolveFromOrigin(message[kNotificationSystemMessageName]);
>               return retval;
>             }
>           }
>   
>           // No message found...
>           return null;
>         }

Comment 1

[:gerard-majax]

Jason, Nicolas checked this and it seems to be a legitimate JS bug: we lack the setrval/retrval when looking at the disassembly of the first code.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: JS shell test case.

Comment 3

[Jason Orendorff [:jorendorff]]

No bug, I'm afraid. It's automatic semicolon insertion. An semicolon is being automatically inserted at the end of the line that says
>               return

According to the standard, you can't have a line break between the 'return' keyword and the value in a return statement. See <http://people.mozilla.org/~jorendorff/es6-draft.html#sec-rules-of-automatic-semicolon-insertion> for all the gruesome details...

Comment 4

[:gerard-majax]

(In reply to Jason Orendorff [:jorendorff] from comment #3)
> No bug, I'm afraid. It's automatic semicolon insertion. An semicolon is
> being automatically inserted at the end of the line that says
> >               return
> 
> According to the standard, you can't have a line break between the 'return'
> keyword and the value in a return statement. See
> <http://people.mozilla.org/~jorendorff/es6-draft.html#sec-rules-of-automatic-
> semicolon-insertion> for all the gruesome details...

Ooops, sorry :).

Well, there's still a bug I should fix.

Comment 5

[:gerard-majax]

As far as I could check the code, I have not been able to find a proper way to test this on Gecko side.

Comment 6

[:gerard-majax]

However, one of the integration tests added as part of bug 967475 exposes the issue.

Comment 7

[:gerard-majax]

Attachment: 0001-Bug-1005093-Fix-return-code-path-of-getNotificationU.patch

Fabrice, sorry for missing this one in the previous patch.

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 8416646 [details] [diff] [review]
0001-Bug-1005093-Fix-return-code-path-of-getNotificationU.patch

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

oh my. stoopid js.

Comment 9

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Alexandre LISSY :gerard-majax from comment #5)
> As far as I could check the code, I have not been able to find a proper way
> to test this on Gecko side.

Honestly, since this is b2g specific code I think Gaia integration tests are what we ultimately want for this. I'd say we add the Gi test from bug 967475 that exposes this issue to this bug. These tests now run on all main branches in TBPL, so this should prevent the regressions we are looking to expose.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/056cbea8ea3a

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/056cbea8ea3a