Closed Bug 1162899 Opened 9 years ago Closed 9 years ago

[B2G Desktop] navigator.mozHasPendingMessage always returns false in in-process app

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox39 --- wontfix
firefox40 --- wontfix
firefox41 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: azasypkin, Assigned: kanru)

References

Details

Attachments

(2 files)

In bug 1156464 we added "mozHasPendingMessage('notification')" call to DOMContentLoaded handler. We tried to create Marionette JS integration test that verifies the case when app is run via notification system message.

But when in B2G Desktop we issue this system message (either via simple Marionette JS tap on notification in Utility Tray or via direct SystemMessageInternal.sendMessage call), "mozHasPendingMessage('notification')" is always false. It doesn't happen on device though.

With preliminary debugging it looks like system message cache is updated later than we call "mozHasPendingMessage('notification')".

Please use test application [1] and Scratchpad script [2] to reproduce issue on B2G Desktop.

There are several ways how to reproduce it, I use the following:

1. Build debug gaia profile (with DEVICE_DEBUG=1 make for example);
2. Start b2g with debugging server enabled (b2g -profile [path_to_debug_profile] -start-debugger-server [port_number]);
3. Open WebIDE in nightly and then Project -> Open packaged app -> path to [1];
4. Install the app (remember manifestURL in case it's different from one used in [2]), and then _close_ it;
5. Switch to "Main Process" -> Scratchpad;
6. Paste script from [2] and press "Run";
7. App will indicate return result of "mozHasPendingMessage('notification')" on "DOMContentLoaded" and "load" events.

Expected result: it always should be "true";
Actual result: it's always "false" for b2g;

Also you can increase "setTimeout" value in index.js from [1] and see that with increased timeout we'll finally get "true" at least in "load" event handler.

[1] https://github.com/azasypkin/app-system-messages
[2] https://github.com/azasypkin/app-system-messages/blob/master/scratchpad-script.js
Hey Kan-Ru,

As we discussed on IRC, could you please check what is going on here? I'm a bit worried that we have race somewhere that can potentially happen on real device.

Thanks!
Flags: needinfo?(kchen)
(In reply to Oleg Zasypkin [:azasypkin] from comment #1)
> Hey Kan-Ru,
> 
> As we discussed on IRC, could you please check what is going on here? I'm a
> bit worried that we have race somewhere that can potentially happen on real
> device.
> 
> Thanks!

I checked and it's because in-process app couldn't use SystemMessageCache. It works on device because all apps are remote.
Flags: needinfo?(kchen)
in-process app could query hasPendingMessage without being blocked.
Assignee: nobody → kchen
Status: NEW → ASSIGNED
Attachment #8603986 - Flags: review?(fabrice)
Attachment #8603986 - Flags: review?(fabrice) → review+
Summary: [B2G Desktop] navigator.mozHasPendingMessage always returns false when queried too early in app startup path → [B2G Desktop] navigator.mozHasPendingMessage always returns false in in-process app
Thanks for the fix, it looks like patch works perfectly for my manually patched b2g! 

Hey Kan-Ru, is there any chance that it can be uplifted to v2.2 as well?
Flags: needinfo?(kchen)
(In reply to Oleg Zasypkin [:azasypkin] from comment #5)
> Thanks for the fix, it looks like patch works perfectly for my manually
> patched b2g! 
> 
> Hey Kan-Ru, is there any chance that it can be uplifted to v2.2 as well?

I'll request approval. However this issue has low risk on v2.2 as we do not use any in-process app on real devices.
Flags: needinfo?(kchen)
https://hg.mozilla.org/mozilla-central/rev/fa102b192965
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment on attachment 8603986 [details] [diff] [review]
Use sync hasPendingMessage for in-process app

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1126119
User impact if declined: The system app or a in-process app can't use mozHasPendingMessages.
Testing completed: manually tested locally.
Risk to taking this patch (and alternatives if risky): little to none
String or UUID changes made by this patch: no
Attachment #8603986 - Flags: approval-mozilla-b2g37?
Hi Kan-Ru,
Could you please add test for the patch? 
Just like to make sure we are at safe side.
Thank you!
Flags: needinfo?(kchen)
Blocks: 1156464
Hey Kan-Ru, it's been 3 working days now, any chance you can look at this soon? This is necessary for the tests to work in bug 1156464 for v2.2.
Comment on attachment 8607455 [details] [diff] [review]
Add tests for mozHasPendingMessages

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

lgtm, but can you move that patch to its own bug carrying r+ ?

::: dom/messages/test/test_hasPendingMessage.html
@@ +11,5 @@
> +
> +    const { utils: Cu, interfaces: Ci, classes: Cc } = Components;
> +    Cu.import("resource://gre/modules/Services.jsm");
> +
> +    SpecialPowers.setAllAppsLaunchable(true);

I don't think you need that since you're not really using the mozApps api.

@@ +37,5 @@
> +        test_frame.setAttribute("remote", isRemote);
> +        test_frame.setAttribute("src", testURL);
> +        test_frame.setAttribute("mozapp", manifestURL);
> +        test_frame.addEventListener("mozbrowserloadend", e => {
> +        });

Why do you need this empty event listener?
Attachment #8607455 - Flags: review?(fabrice) → review+
(In reply to Fabrice Desré [:fabrice] from comment #12)
> Comment on attachment 8607455 [details] [diff] [review]
> Add tests for mozHasPendingMessages
> 
> Review of attachment 8607455 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm, but can you move that patch to its own bug carrying r+ ?
> 
> ::: dom/messages/test/test_hasPendingMessage.html
> @@ +11,5 @@
> > +
> > +    const { utils: Cu, interfaces: Ci, classes: Cc } = Components;
> > +    Cu.import("resource://gre/modules/Services.jsm");
> > +
> > +    SpecialPowers.setAllAppsLaunchable(true);
> 
> I don't think you need that since you're not really using the mozApps api.

Indeed. Removed.

> @@ +37,5 @@
> > +        test_frame.setAttribute("remote", isRemote);
> > +        test_frame.setAttribute("src", testURL);
> > +        test_frame.setAttribute("mozapp", manifestURL);
> > +        test_frame.addEventListener("mozbrowserloadend", e => {
> > +        });
> 
> Why do you need this empty event listener?

A leftover..
Blocks: 1166580
Comment on attachment 8603986 [details] [diff] [review]
Use sync hasPendingMessage for in-process app

Approving as Kanru adding UT in bug 1166580.
Thanks KanRu!
Attachment #8603986 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.