Regression: Contact API usage is being prompt on webpages

VERIFIED FIXED in Firefox 32, Firefox OS v2.0

Status

()

Firefox for Android
General
--
critical
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: aaronmt, Assigned: fabrice)

Tracking

({regression, reproducible})

Trunk
Firefox 33
All
Android
regression, reproducible
Points:
---

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox31 unaffected, firefox32+ verified, firefox33 verified, b2g-v2.0 fixed, b2g-v2.1 fixed, fennec32+)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
Trunk Nightly (07/10) and Aurora (07/10) (according to esawin) have contacts API enabled (accidentally?)

I can confirm getting blasted with prompts on e.g, drudgereport
Could be related to bug 1024513, which landed on Nightly and Aurora
Flags: needinfo?(fabrice)
Created attachment 8454034 [details]
fennec-contacts-request.png

Example
(Assignee)

Comment 3

4 years ago
Hm, my patch changed the permission handling, but I didn't enable the api itself...
Flags: needinfo?(fabrice)
(Reporter)

Updated

4 years ago
tracking-fennec: --- → ?
I verified that backing out bug 1024513 fixes this issue.
Blocks: 1024513
I tried using a patch in bug 1037030 (https://bug1037030.bugzilla.mozilla.org/attachment.cgi?id=8454268) but it did not fix the issue.

I tried a suggestion from Brad Lassey:

>     if (permValue == Ci.nsIPermissionManager.ALLOW_ACTION) {
>       aAllowCallback();
>       return;
>     } else if (permValue == Ci.nsIPermissionManager.DENY_ACTION) {
>       aCancelCallback();
>+      return;
>     }

Which might be needed regardless, but it did not fix the issue.
I think I found the issue. The old PermissionPromptHelper.jsm file used this check:

    if (permValue == Ci.nsIPermissionManager.DENY_ACTION ||
        permValue == Ci.nsIPermissionManager.UNKNOWN_ACTION) {
      aCallbacks.cancel();
      return;
    }


The UNKNOWN_ACTION was dropped in the refactor. Adding it back fixes the Contacts API prompt issue:

     if (permValue == Ci.nsIPermissionManager.ALLOW_ACTION) {
       aAllowCallback();
       return;
-    } else if (permValue == Ci.nsIPermissionManager.DENY_ACTION) {
+    } else if (permValue == Ci.nsIPermissionManager.DENY_ACTION ||
+               permValue == Ci.nsIPermissionManager.UNKNOWN_ACTION) {
       aCancelCallback();
+      return;
     }
Severity: normal → critical
Summary: Contact API (accidentally?) enabled → Contact API (accidentally?) enabled for Webpages
Just a note: I do not know if my proposed change in comment 6 is the "correct" thing to do. I was just noting that it did fix the issue.

I need Fabrice to weigh in on this.
Severity: critical → normal
Flags: needinfo?(fabrice)
Summary: Contact API (accidentally?) enabled for Webpages → Contact API (accidentally?) enabled
Severity: normal → critical
Summary: Contact API (accidentally?) enabled → Contact API (accidentally?) enabled for Webpages
(Reporter)

Updated

4 years ago
status-firefox31: --- → unaffected
status-firefox32: --- → affected
status-firefox33: --- → affected
tracking-firefox32: --- → ?
(Assignee)

Comment 8

4 years ago
(In reply to Mark Finkle (:mfinkle) from comment #7)
> Just a note: I do not know if my proposed change in comment 6 is the
> "correct" thing to do. I was just noting that it did fix the issue.
> 
> I need Fabrice to weigh in on this.

Comment 6 is correct, I messed up the refactoring :(
Flags: needinfo?(fabrice)
(Reporter)

Updated

4 years ago
Keywords: regression, reproducible
Summary: Contact API (accidentally?) enabled for Webpages → Regression: Contact API usage is being prompt on webpages
(Reporter)

Comment 9

4 years ago
This needs to be fixed before beta.
Created attachment 8455344 [details] [diff] [review]
contacts-fix v0.1

I was hoping someone else would be able to get to this, but Aaron is right. This needs to be fixed ASAP.

Fabrice - Is this fix good enough?
Assignee: nobody → mark.finkle
Attachment #8455344 - Flags: review?(fabrice)
(Assignee)

Updated

4 years ago
Attachment #8455344 - Flags: review?(fabrice) → review+
(Reporter)

Comment 16

4 years ago
Any update here?
Status: NEW → ASSIGNED
(Assignee)

Comment 17

4 years ago
(In reply to Aaron Train [:aaronmt] from comment #16)
> Any update here?

Yes, I need to fix the last M8 orange in debug emulators: https://tbpl.mozilla.org/?tree=Try&rev=8c37148c0215
tracking-firefox32: ? → +
(Assignee)

Comment 18

4 years ago
And of course I can't reproduce locally :(
This bug needs to be fixed before we can ship a beta for Fx32. That means we need to backout the original patches that caused the regression, or we land Fabrice's fix and live with the M8 orange in debug emulators.

I vote for the latter.
tracking-fennec: ? → 32+
(Assignee)

Comment 20

4 years ago
It seems that debug contact tests are flaky anyway, so I disabled the offending ones. Green try at 

https://tbpl.mozilla.org/?tree=Try&rev=c190d6dec7d3

landed: https://hg.mozilla.org/integration/b2g-inbound/rev/0144a56aef70
Assignee: mark.finkle → fabrice
https://hg.mozilla.org/mozilla-central/rev/0144a56aef70
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
https://hg.mozilla.org/releases/mozilla-aurora/rev/9779e1a3c05b
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
status-firefox32: affected → fixed
status-firefox33: affected → fixed
(Reporter)

Updated

4 years ago
Status: RESOLVED → VERIFIED
status-firefox32: fixed → verified
status-firefox33: fixed → verified
You need to log in before you can comment on or make changes to this bug.