B2G can't launch app since it's seen as an XP_UNIX by WebappOSUtils.jsm

VERIFIED FIXED in Firefox 16

Status

()

Core
DOM: Apps
--
blocker
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: etienne, Assigned: etienne)

Tracking

({regression})

Trunk
mozilla17
All
Linux
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox16 verified, firefox17 verified)

Details

(Whiteboard: [blocking-webrtandroid1+])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
So we don't go through the |Services.obs.notifyObservers(this, "webapps-launch", JSON.stringify(aData))| path.
(Assignee)

Comment 1

5 years ago
Created attachment 642522 [details] [diff] [review]
Workarround
#ifndef ANDROID will not work on the desktop build.
(Assignee)

Comment 3

5 years ago
(In reply to Vivien Nicolas (:vingtetun) from comment #2)
> #ifndef ANDROID will not work on the desktop build.

Yep, it doesn't.

Happy to update the patch if there is a better way.

Comment 4

5 years ago
How about #ifdef MOZ_WIDGET_GONK?

Comment 5

5 years ago
r=me with that hack, until we have a better fix
(Assignee)

Comment 6

5 years ago
Created attachment 642583 [details] [diff] [review]
Patch proposal

This is a more stable fix for B2G desktop *and* on the phone.
Thanks to :vingtetun for the pointer.
Attachment #642522 - Attachment is obsolete: true
Attachment #642583 - Flags: review?(gal)

Comment 7

5 years ago
Comment on attachment 642583 [details] [diff] [review]
Patch proposal

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

Nice. r=me but please add a comment explaining the event flow.
Attachment #642583 - Flags: review?(gal) → review+
Doesn't this patch revert the change for Firefox?
Comment on attachment 642583 [details] [diff] [review]
Patch proposal

>+++ b/webapprt/WebappsHandler.jsm
>+Cu.import("resource://gre/modules/WebappOSUtils.jsm");
..
>       case "webapps-launch":
>+        WebappOSUtils.launch(data);

This change would also need to made for Firefox I believe?

http://mxr.mozilla.org/mozilla-central/source/browser/modules/webappsUI.jsm#36
Attachment #642583 - Flags: review?(felipc)
Blocks: 772600
Keywords: regression
(Assignee)

Comment 10

5 years ago
(In reply to Edward Lee :Mardak from comment #8)
> Doesn't this patch revert the change for Firefox?

Either https://mxr.mozilla.org/mozilla-central/source/browser/modules/webappsUI.jsm#36 was dead code or this patch will "fix" firefox too.
It seems like the patch that landed with bug 772600 was lacking in various ways including removing the dead code from Firefox. I'm not sure why there are 5 bugs open for the same feature, but originally it was filed as bug 745924 then split into bug 763740, bug 763786, and bug 763789; and then was implemented in bug 772600 -- all of those were targeting Firefox 16.

I'll double check with drivers to figure out (hopefully within an hour) for how we can make sure b2g isn't broken while fixing the intended platforms and runtimes.
(Assignee)

Comment 12

5 years ago
Created attachment 642604 [details] [diff] [review]
Patch v2
Attachment #642583 - Attachment is obsolete: true
Attachment #642583 - Flags: review?(felipc)
Attachment #642604 - Flags: review?(edilee)
(Assignee)

Comment 13

5 years ago
(In reply to Edward Lee :Mardak from comment #11)
> I'll double check with drivers to figure out (hopefully within an hour) for
> how we can make sure b2g isn't broken while fixing the intended platforms
> and runtimes.

Thanks for tracking this!

In the meantime here is a patch proposal ensuring that we do not have side effects outside of the b2g runtime.

Updated

5 years ago
blocking-basecamp: --- → ?
Comment on attachment 642604 [details] [diff] [review]
Patch v2

Switching r?felipe for browser code, although I don't think it'll get r+d with keeping the pinned tab launch behavior.

>+++ b/browser/modules/webappsUI.jsm
>       case "webapps-launch":
>+        WebappOSUtils.launch(data);
This line looks fine although I can't r+ for browser/.

>         DOMApplicationRegistry.getManifestFor(data.origin, (function(aManifest) {
>           if (!aManifest)
>             return;
>           let manifest = new DOMApplicationManifest(aManifest, data.origin);
>           this.openURL(manifest.fullLaunchPath(), data.origin);
>         }).bind(this));
We actually want to get rid of this code because we don't want a native launch /and/ a pinned tab launch.

>+++ b/toolkit/webapps/WebappOSUtils.jsm
>@@ -1,15 +1,21 @@
>+/* Only used for webapprt and browser runtimes
>+   webapps-launch events are still systematically sent. */
Not sure if this comment is necessary or exactly what you intend with "systematically sent."

>+const Cu = Components.utils;
>+Cu.import("resource://gre/modules/Services.jsm");
Thanks for fixing this.
Attachment #642604 - Flags: review?(felipc)
Attachment #642604 - Flags: review?(edilee)
Attachment #642604 - Flags: feedback+
felipe: To help with the review, this patch switches back to always notifying, and we have 3 consumers for the notification:

1) b2g: works as it did before
2) firefox: switch to using os utils launching (and remove pinned tab launch)
3) webapprt: keep the os util launching
(Assignee)

Comment 16

5 years ago
(In reply to Edward Lee :Mardak from comment #14)
> Comment on attachment 642604 [details] [diff] [review]
> Patch v2
> 
> Switching r?felipe for browser code, although I don't think it'll get r+d
> with keeping the pinned tab launch behavior.
> 
> >+++ b/browser/modules/webappsUI.jsm
> >       case "webapps-launch":
> >+        WebappOSUtils.launch(data);
> This line looks fine although I can't r+ for browser/.
> 
> >         DOMApplicationRegistry.getManifestFor(data.origin, (function(aManifest) {
> >           if (!aManifest)
> >             return;
> >           let manifest = new DOMApplicationManifest(aManifest, data.origin);
> >           this.openURL(manifest.fullLaunchPath(), data.origin);
> >         }).bind(this));
> We actually want to get rid of this code because we don't want a native
> launch /and/ a pinned tab launch.

Make sense.
I'll remove the dead code call, but I'll let somebody more aware of the situation remove the openURL function.

> 
> >+++ b/toolkit/webapps/WebappOSUtils.jsm
> >@@ -1,15 +1,21 @@
> >+/* Only used for webapprt and browser runtimes
> >+   webapps-launch events are still systematically sent. */
> Not sure if this comment is necessary or exactly what you intend with
> "systematically sent."

Don't want to let somebody think that it's a good place to change something canonical to all webapps-launch.
Happy to reformulate/remove.

> 
> >+const Cu = Components.utils;
> >+Cu.import("resource://gre/modules/Services.jsm");
> Thanks for fixing this.

np.
blocking-basecamp: ? → ---
(Assignee)

Comment 17

5 years ago
Created attachment 642626 [details] [diff] [review]
Patch v3
Attachment #642604 - Attachment is obsolete: true
Attachment #642604 - Flags: review?(felipc)
Attachment #642626 - Flags: review?(felipc)
Attachment #642626 - Flags: review?(felipc) → review+
this is a blocker for b2g daily builds.  I cannot launch any apps on the phone. 

we need this fixed in m-c asap.
Severity: normal → blocker
blocking-basecamp: --- → ?
Assignee: nobody → etienne
Created attachment 642654 [details] [diff] [review]
for checkin

Updated patch to be against mozilla-central (no ActivitiesService.jsm) and updated commit message.
Attachment #642626 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/c918ff2f0994
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 21

5 years ago
Thanks for the quick fix.

Updated

5 years ago
Whiteboard: [qa+]

Updated

5 years ago
QA Contact: jsmith
Verified fix on 7-17-2012 daily build
Status: RESOLVED → VERIFIED

Updated

5 years ago
QA Contact: jsmith → tchung
Whiteboard: [qa+]
Moving to Core to request aurora-approval
Assignee: etienne → nobody
Component: General → DOM
Product: Boot2Gecko → Core
QA Contact: tchung
Target Milestone: --- → mozilla17
Version: unspecified → Trunk
Assignee: nobody → etienne
QA Contact: tchung
Comment on attachment 642654 [details] [diff] [review]
for checkin

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 772600

User impact if declined: None if the 16 branch won't be used by B2G or Android. (Bug does not happen on desktop, but it'd still be good to have the code in 16 and trunk to match).
 If used by Android then launching apps from inside the about:apps dashboard (not the homescreen) won't work.  

Testing completed (on m-c, etc.): landed on m-c at the beginning of the week and verified to have fixed the problem

Risk to taking this patch (and alternatives if risky): very little
String or UUID changes made by this patch: none
Attachment #642654 - Flags: approval-mozilla-aurora?

Updated

5 years ago
status-firefox16: --- → affected

Updated

5 years ago
Component: DOM → DOM: Mozilla Extensions
blocking-basecamp: ? → +
Comment on attachment 642654 [details] [diff] [review]
for checkin

Low risk and we definitely don't want to break app launching on Android, approving.
Attachment #642654 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Component: DOM: Mozilla Extensions → DOM: Apps

Updated

5 years ago
Duplicate of this bug: 777843

Updated

5 years ago
Blocks: 766259

Updated

5 years ago
Whiteboard: [blocking-webrtandroid1+]
We still need this uplift in order to get apps working on FF 16. Can someone get this uplifted?
https://hg.mozilla.org/releases/mozilla-aurora/rev/27235727dfbf
status-firefox16: affected → fixed

Updated

5 years ago
Whiteboard: [blocking-webrtandroid1+] → [blocking-webrtandroid1+], [qa+]
Aaron - Can you verify this is fixed on Aurora when a new build comes out for Aurora on Firefox for Android?

Updated

5 years ago
status-firefox16: fixed → verified
status-firefox17: --- → verified

Updated

5 years ago
Whiteboard: [blocking-webrtandroid1+], [qa+] → [blocking-webrtandroid1+]
You need to log in before you can comment on or make changes to this bug.