1037128 - Regression: Contact API usage is being prompt on webpages

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

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

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Could be related to bug 1024513, which landed on Nightly and Aurora

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: fennec-contacts-request.png

Example

Comment 3

[[:fabrice] Fabrice Desré]

Hm, my patch changed the permission handling, but I didn't enable the api itself...

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

I verified that backing out bug 1024513 fixes this issue.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

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;
     }

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

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 8

[[:fabrice] Fabrice Desré]

(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 :(

Comment 9

[Aaron Train [:aaronmt]]

This needs to be fixed before beta.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: 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?

Comment 11

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/1a9546b1b79d

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest failures.
https://hg.mozilla.org/integration/b2g-inbound/rev/ae4192ba2824

https://tbpl.mozilla.org/php/getParsedLog.php?id=43760187&tree=B2g-Inbound

Comment 13

[[:fabrice] Fabrice Desré]

fixed at https://tbpl.mozilla.org/?tree=Try&rev=e7073f9ba795

https://hg.mozilla.org/integration/b2g-inbound/rev/a6ecd3131b8a

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for Gu failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=43773875&tree=B2g-Inbound

https://hg.mozilla.org/integration/b2g-inbound/rev/510ce894eaca

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Wes Kocher (:KWierso) from comment #14)
> Backed out for Gu failures:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=43773875&tree=B2g-Inbound
> 
> https://hg.mozilla.org/integration/b2g-inbound/rev/510ce894eaca

There was also this mochitest failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=43777201&tree=B2g-Inbound

Comment 16

[Aaron Train [:aaronmt]]

Any update here?

Comment 17

[[:fabrice] Fabrice Desré]

(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

Comment 18

[[:fabrice] Fabrice Desré]

And of course I can't reproduce locally :(

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 20

[[:fabrice] Fabrice Desré]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0144a56aef70

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9779e1a3c05b