Galaxy S4 - HelperApps PageAction shows on pages that end in .html or .htm

RESOLVED FIXED in Firefox 28, Firefox OS v1.3

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

26 Branch
Firefox 30
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox28+ fixed, firefox29 fixed, firefox30 fixed, b2g-v1.3 fixed, fennec+)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
The HelperApps page action is showing on nyt stories for me. clicking it launches the video player app, which then says "Cannot play video".
(Assignee)

Updated

5 years ago
Blocks: 899376
Which article? I don't get any page actions at all on a bunch of articles (I have the native app installed).
(Assignee)

Comment 2

5 years ago
http://www.nytimes.com/2013/09/04/us/politics/obama-administration-presses-case-on-syria.html?hp

shows for me. This is an S3 and it's "stock" video player.
Created attachment 811802 [details]
Screenshot

A user just reported the same, but for Boing Boing, on current Aurora. Also with video player. Couldn't reproduce on HTC One.
OS: Linux → Android
Hardware: x86_64 → All
Version: Trunk → Firefox 26

Comment 4

5 years ago
Re rnewman's comment above, I'm the user, I'm on a Galaxy S2 (SPH-D710) running stock 4.1.2, Aurora 26.0a2 (2013-09-29), and I don't have a BoingBoing app (if there is one). I've had the same experience on other sites which I can't recall at the moment, but I'll keep an eye out for it. The URL I was at was the mobile version of http://boingboing.net/2013/09/29/32-helpful-everyday-tips.html
(Assignee)

Comment 5

5 years ago
Yeah, I suspect Samsung has some strange regex in the video player manifest that's incorrectly triggering on some pages. I'll try to test and see if I can narrow it down.

Hacky, but maybe we can blocklist the video player apps for this unless we're showing a video.

Comment 6

5 years ago
Also happened for me with a Wikipedia page - I have the Wikipedia app installed, so I was expecting this to launch that.  Which is what happens if I open a wikipedia page in Chrome.

This is on a Nexus 5, so it's not Samsung-specific.
(Assignee)

Comment 7

4 years ago
Created attachment 8374475 [details] [diff] [review]
Patch

This checks for handlers for http://www.example.com/index.html and avoids showing the icon form them as well. I picked that on someone's suspicion that the Video Player was checking for things that end with 'html'. It works for me on my S3 and feels generic enough.
Attachment #8374475 - Flags: review?(margaret.leibovic)
does it catch .htm as well?
Duplicate of this bug: 970574
Summary: HelperApps PageAction shows on NYT articles. Launches video player, then fails → Galaxy S4 - HelperApps PageAction shows on pages that end in .html or .htm
(Assignee)

Comment 10

4 years ago
Do we think we need both? TBH, this will strip the Samsung Video Player from all results. Other video players you might have installed will still appear. I'm not sure if that's acceptable or not. We're not stripping it BECAUSE its a Video player. We're stripping it because its pretending to be a browser (and we'd strip any other apps pretending to be browsers as well).
All I care about is making sure the site listed at 970574 comment 0 which ends in .htm is not displaying an Android helper action and that mp4 and other video content can still be played by the Samsung video player.
Comment on attachment 8374475 [details] [diff] [review]
Patch

nifty, but 'var' ? :)
(In reply to Kevin Brosnan [:kbrosnan] from comment #11)
> All I care about is making sure the site listed at 970574 comment 0 which
> ends in .htm is not displaying an Android helper action and that mp4 and
> other video content can still be played by the Samsung video player.

I assume the same "bad app" would also look for .html as well as .htm in it's AndroidManifest.xml so we'd remove it for any "http://", which is what Wes' code is really doing.
(Assignee)

Comment 14

4 years ago
So to do this "righter" I think we'll need to try a little harder. We can use this to get a list of ".html" handlers, and then only run that list against pages that use the .html extension. That feels even more hacky than this, but it would allow the Samsung Video Player to still appear when you load a video file, and wouldn't prevent the Github app from appearing on github.com/index.html.

Comment 15

4 years ago
Comment on attachment 8374475 [details] [diff] [review]
Patch

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

::: mobile/android/modules/HelperApps.jsm
@@ +36,5 @@
>    get defaultHttpHandlers() {
>      delete this.defaultHttpHandlers;
> +    this.defaultHttpHandlers = this.getAppsForProtocol("http");
> +
> +    var htmlHandlers = this.getAppsForUri(Services.io.newURI("http://www.example.com/index.html", null, null));

I'm trying to familiarize myself with HelperApps here... why would getAppsForUri return more things than getAppsForProtocol in this case?
(Assignee)

Comment 16

4 years ago
An Activity can register itself for a scheme or for a uri (even a regex of a uri). So for instance the GitHub app registers for www.github.com urls. It won't show up if you just ask for anyone who wants http schemes (browsers generally), but it will show up if you want something with a github.com url.

In this case, we think the Samsung Video player registers itself for any files ending in .htm or .html. We'd like to filter it (and any other apps that are registering for very generic things) out of our list. Maybe we'd be better to try filtering things that look for text/html out. I'll try that...
Assignee: nobody → wjohnston
tracking-fennec: --- → +
(Assignee)

Comment 17

4 years ago
Created attachment 8375783 [details] [diff] [review]
Patch

Updated. This checks the extension of the uri passed in and only does the filtering if its "htm" or "html".
Attachment #8374475 - Attachment is obsolete: true
Attachment #8374475 - Flags: review?(margaret.leibovic)
Attachment #8375783 - Flags: review?(margaret.leibovic)
Did a quick check on the S4, php and aspx are not affected. Anything else common that should be checked while we are still in the implementation phase?
(Assignee)

Comment 19

4 years ago
Created attachment 8375848 [details] [diff] [review]
Patch
Attachment #8375783 - Attachment is obsolete: true
Attachment #8375783 - Flags: review?(margaret.leibovic)
Attachment #8375848 - Flags: review?(margaret.leibovic)

Comment 20

4 years ago
Comment on attachment 8375848 [details] [diff] [review]
Patch

This looks like a patch on top of the old patch... post a folded one?

Comment 21

4 years ago
Comment on attachment 8375848 [details] [diff] [review]
Patch

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

::: mobile/android/modules/HelperApps.jsm
@@ +50,1 @@
>      }, this);

Don't you need to return this.htmlHandlers here?

@@ +102,5 @@
>          }, this);
>        }
>  
> +      if (flags.filterHtml) {
> +        var ext = /\.([^\?#]*)/.exec(uri.path)[1];

Use let instead of var, and document what this regex is doing.

@@ +103,5 @@
>        }
>  
> +      if (flags.filterHtml) {
> +        var ext = /\.([^\?#]*)/.exec(uri.path)[1];
> +        if (ext == "html" || ext === "htm") {

Why one double equals and one triple? I'll put on my mcomella hat and just say use triple equals everywhere :)

@@ +106,5 @@
> +        var ext = /\.([^\?#]*)/.exec(uri.path)[1];
> +        if (ext == "html" || ext === "htm") {
> +          apps = apps.filter(app => {
> +            return app.name && !this.defaultHtmlHandlers[app.name];
> +          });

I know fat arrows are cool, but it might be nice to just be consistent with the filter call up above.

Comment 22

4 years ago
Comment on attachment 8375848 [details] [diff] [review]
Patch

Looking to see my comments addressed, and a folded version of the patch.
Attachment #8375848 - Flags: review?(margaret.leibovic) → feedback+
(Assignee)

Comment 23

4 years ago
Created attachment 8376039 [details] [diff] [review]
Patch v2

I don't know whats happening to my head. Folded the two patches, and fixed up the comments.
Attachment #8375848 - Attachment is obsolete: true
Attachment #8376039 - Flags: review?(margaret.leibovic)
Comment on attachment 8376039 [details] [diff] [review]
Patch v2


>diff --git a/mobile/android/modules/HelperApps.jsm b/mobile/android/modules/HelperApps.jsm

>+  get htmlHandlers() {

>+  getAppsForUri: function getAppsForUri(uri, flags = { filterHttp: true, filterHtml: true }, callback) {

>+          apps = apps.filter(app => {
>+            return app.name && !this.defaultHtmlHandlers[app.name];

You use "htmlHandlers" and "defaultHtmlHandlers". I like "defaultHtmlHandlers".
(Assignee)

Comment 25

4 years ago
Created attachment 8376085 [details] [diff] [review]
Patch

Good catch. Thanks.
Attachment #8376039 - Attachment is obsolete: true
Attachment #8376039 - Flags: review?(margaret.leibovic)
Attachment #8376085 - Flags: review?(margaret.leibovic)

Comment 26

4 years ago
Comment on attachment 8376085 [details] [diff] [review]
Patch

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

Nice.

::: mobile/android/modules/HelperApps.jsm
@@ +82,5 @@
>  
>      return results;
>    },
>  
> +  getAppsForUri: function getAppsForUri(uri, flags = { filterHttp: true, filterHtml: true }, callback) {

I know you're following what already exists here, but you don't really need these default values if you just default to setting them as true below.
Attachment #8376085 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/962ba8d1a8f4
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
(Assignee)

Comment 29

4 years ago
Comment on attachment 8376085 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 899376
User impact if declined: Android icon shows when it shouldn't on some phones
Testing completed (on m-c, etc.): Landed on mc last week. Seems stable.
Risk to taking this patch (and alternatives if risky): Low risk. Worst case we don't show the HelperApps button sometimes when we should. We've already shipped this feature to users with this bug, so uplift to beta might be a stretch, but its a highly visible bug.
String or IDL/UUID changes made by this patch: None.
Attachment #8376085 - Flags: approval-mozilla-beta?
Attachment #8376085 - Flags: approval-mozilla-aurora?
Attachment #8376085 - Flags: approval-mozilla-beta?
Attachment #8376085 - Flags: approval-mozilla-beta+
Attachment #8376085 - Flags: approval-mozilla-aurora?
Attachment #8376085 - Flags: approval-mozilla-aurora+
status-firefox28: --- → affected
status-firefox29: --- → affected
status-firefox30: --- → fixed
tracking-firefox28: --- → +
Needs some love for uplift.
Flags: needinfo?(wjohnston)
Keywords: branch-patch-needed
FWIW, bug 946344 is what's causing the conflicts.
(Assignee)

Comment 32

4 years ago
Created attachment 8378855 [details] [diff] [review]
Follow up Patch

I saw this spitting errors tonight and realized that calling getAppsForUri actually calls back into this.defaultHtmlHandlers now which is undefined at that point.

The redefines it before that call so that it is defined (just empty). It also disables the filter for that query though.
Attachment #8378855 - Flags: review?(margaret.leibovic)
Flags: needinfo?(wjohnston)
Still needinfo? on the branch patch for uplift :)
Flags: needinfo?(wjohnston)
What simple tests could we write for this? It's a JSM, so that might make a simple test easier to write.

Comment 35

4 years ago
Comment on attachment 8378855 [details] [diff] [review]
Follow up Patch

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

::: mobile/android/modules/HelperApps.jsm
@@ +43,5 @@
>    get defaultHtmlHandlers() {
>      delete this.defaultHtmlHandlers;
> +    this.defaultHtmlHandlers = {};
> +    let handlers = this.getAppsForUri(Services.io.newURI("http://www.example.com/index.html", null, null), {
> +      filterHtml: false

The important change is the filterHtml: false, so you don't need to move where this.defaultHtmlHandlers is set, right?

Also, this circular dependency is kinda tricky :/
Attachment #8378855 - Flags: review?(margaret.leibovic) → review+

Comment 36

4 years ago
(In reply to Mark Finkle (:mfinkle) from comment #34)
> What simple tests could we write for this? It's a JSM, so that might make a
> simple test easier to write.

Can we have some dumb unit-style tests that just call getAppsForUri? I suppose the problem is we don't know what apps are around on devices in a test environment.

It would be cool if we could do something like intercept the "Intent:GetHandlers" message in a test, and return some mock data, but I'm not sure how we would do that.
Duplicate of this bug: 975551
(Assignee)

Comment 40

4 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/718fe43eeed1
status-firefox28: affected → fixed
(Assignee)

Comment 41

4 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/87c4e3a46602
status-firefox29: affected → fixed
Keywords: branch-patch-needed
status-b2g-v1.3: --- → fixed

Updated

4 years ago
Blocks: 1115503
You need to log in before you can comment on or make changes to this bug.