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

RESOLVED FIXED in Firefox 41, Firefox OS v2.2

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: azasypkin, Assigned: kanru)

Tracking

unspecified
mozilla41
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox39 wontfix, firefox40 wontfix, firefox41 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(2 attachments)

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

Comment 1

3 years ago
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)
(Assignee)

Comment 2

3 years ago
(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)
(Assignee)

Comment 3

3 years ago
Created attachment 8603986 [details] [diff] [review]
Use sync hasPendingMessage for in-process app

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+
(Assignee)

Updated

3 years ago
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
(Reporter)

Comment 5

3 years ago
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)
(Assignee)

Comment 6

3 years ago
(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
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Assignee)

Comment 8

3 years ago
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?

Comment 9

3 years ago
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)

Updated

3 years ago
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.
(Assignee)

Comment 11

3 years ago
Created attachment 8607455 [details] [diff] [review]
Add tests for mozHasPendingMessages

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe645edefc66
Flags: needinfo?(kchen)
Attachment #8607455 - Flags: review?(fabrice)
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+
(Assignee)

Comment 13

3 years ago
(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..
(Assignee)

Updated

3 years ago
Blocks: 1166580

Comment 14

3 years ago
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+

Updated

3 years ago
status-b2g-v2.2: --- → affected
status-b2g-master: --- → fixed
status-firefox39: --- → wontfix
status-firefox40: --- → wontfix
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/cc237ec72655
status-b2g-v2.2: affected → fixed
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.