701804 - Pageactions this._handlers is null

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Extension Compatibility, defect)

Description

[Axel Nennker]

Attachment: openid.xpi more debug output than the AMO version

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0) Gecko/20100101 Firefox/8.0
Build ID: 20111104165243

Steps to reproduce:

Install OpenID for Firefox (attached) in Firefox Beta on Android and clicked the openid icon in the pageactions. 
https://addons.mozilla.org/en-US/firefox/addon/openid-for-firefox/
This happens in Firefox Beta (Gecko/20111109 Firefox/9.0).
This works in the current Firefox Mobile (Gecko/20111104) on Android.


Actual results:

Nothing. Pageactions closed because PageActions.Register threw this._handlers is null.


Expected results:

The user's openid should have been entered into the openid_url input field.

The console output shows the error message "TypeError: this._handlers is null" in the function that calls PageActions.register.

Comment 1

[Axel Nennker]

Gecko/20111114 Firefox/11.0a1 has this bug also.
An xpi to test for yourself is here: version 1.2.10
https://addons.mozilla.org/en-US/firefox/addon/openid-for-firefox/versions/
The version 1.2.10 works with the current release of Firefox and only one feature (PageAction) of the addon does not work in Beta and Nighly.

Comment 2

[tyunshan]

I got the same issue.  Find this one before open a new bug.

In fennec 10.x, such as fennec-10.0a2, the PageActions.register function does not work. In try/catch, It always report "TypeError: this._handlers is null".   Please support this issue.

In my other test, PageActions.register does work at fennec 8.x such as fennec-8.0a1

Comment 3

[Matt Brubeck (:mbrubeck)]

This is a regression from bug 687729.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: patch

A simple one-line fix.  We need this on Aurora and Beta too.

Comment 5

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/753ed712f1be

Comment 6

[Matt Brubeck (:mbrubeck)]

Comment on attachment 580977 [details] [diff] [review]
patch

Requesting approval for Aurora 10 and Beta 9.  This is a one-line mobile-only fix for a regression in update 9 that breaks some extensions.  The fix is very safe and has little to no potential for regressions or perf impact.

Note: The alternate, less-desirable solution would be for the author of each affected add-on to add "if (PageActions._handlers === null) PageActions.init();" before their first PageActions.register call, until a fixed version of Firefox is released.

Comment 7

[Alex Keybl [:akeybl]]

Comment on attachment 580977 [details] [diff] [review]
patch

[Triage Comment]
Let's take this low-risk mobile-only fix on aurora/beta. Please land asap to make it into our final beta.

Comment 8

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-beta/rev/f9fedb00497d
https://hg.mozilla.org/releases/mozilla-aurora/rev/04043d9a960f

Comment 9

[Axel Nennker]

Attachment: Test Case

Thank you for fixing this. Although I don't neither what's wrong with the previous code in common-ui.js not why the solution should work.

I copied the definition of PageActions from http://mxr.mozilla.org/mobile-browser/source/chrome/content/common-ui.js and put it into the attached file to have an easy test. If you load PageActions.html into Firefox (Desktop) then the output is "aId" and this is what I expect.
This test does not test this bug nor your solution.

Could you please explain why it is even possible that _handlers can not be initialized? Calling PageActions.init does not set _handlers either. Why does your fix fix this problem?

Thanks.

Comment 10

[Axel Nennker]

Answering my own question; Sorry.
I looked at the wrong file. The correct file is
https://hg.mozilla.org/releases/mozilla-beta/file/f9fedb00497d/mobile/chrome/content/PageActions.js
not 
http://mxr.mozilla.org/mobile-browser/source/chrome/content/common-ui.js

common-ui.js initialized _handlers like so:
var PageActions = {
  _handlers : [],
};

while PageActions.js does initialize _handlers in init:
var PageActions = {
  _handlers : null,
  init : function() {
    if (this._handlers) return;
    this._handlers = [];
  }
};

Isn't the 'usual' Mozilla coding pattern to define a getter for things like _handlers and initialize them on first use?

Comment 11

[Catalin Suciu [:csuciu]]

Verified fixed on Firefox9 Beta6.
Mozilla /5.0 (Android;Linux armv7l;rv:9.0) Gecko/20111212 Firefox/9.0 Fennec/9.0 
Device: HTC Desire (Android 2.2)

Comment 12

[Matt Brubeck (:mbrubeck)]

(In reply to Axel Nennker from comment #10)
> Isn't the 'usual' Mozilla coding pattern to define a getter for things like
> _handlers and initialize them on first use?

In this case, _handlers is serving a second purpose as a flag to tell whether init() has already been called.  This was probably a bad idea; an explicit flag would have been better and would have avoided this bug.  (I would normally submit a patch to clean this up, but we're trying to minimize changes to XUL fennec while our QA and development resources are focused on native Android fennec.)