Last Comment Bug 701804 - Pageactions this._handlers is null
: Pageactions this._handlers is null
Status: RESOLVED FIXED
: regression, verified-beta
Product: Fennec Graveyard
Classification: Graveyard
Component: Extension Compatibility (show other bugs)
: Firefox 9
: ARM Android
: -- normal (vote)
: Firefox 11
Assigned To: Matt Brubeck (:mbrubeck)
:
Mentors:
Depends on:
Blocks: 687729
  Show dependency treegraph
 
Reported: 2011-11-11 11:37 PST by Axel Nennker
Modified: 2011-12-14 09:58 PST (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
openid.xpi more debug output than the AMO version (72.08 KB, application/octet-stream)
2011-11-11 11:37 PST, Axel Nennker
no flags Details
patch (980 bytes, patch)
2011-12-12 11:11 PST, Matt Brubeck (:mbrubeck)
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review
Test Case (7.41 KB, text/plain)
2011-12-13 03:56 PST, Axel Nennker
no flags Details

Description Axel Nennker 2011-11-11 11:37:59 PST
Created attachment 573870 [details]
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 2011-11-15 07:57:26 PST
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 2011-12-12 01:12:05 PST
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) 2011-12-12 11:06:43 PST
This is a regression from bug 687729.
Comment 4 Matt Brubeck (:mbrubeck) 2011-12-12 11:11:32 PST
Created attachment 580977 [details] [diff] [review]
patch

A simple one-line fix.  We need this on Aurora and Beta too.
Comment 5 Matt Brubeck (:mbrubeck) 2011-12-12 12:26:57 PST
https://hg.mozilla.org/mozilla-central/rev/753ed712f1be
Comment 6 Matt Brubeck (:mbrubeck) 2011-12-12 12:31:07 PST
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] 2011-12-12 13:34:04 PST
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 9 Axel Nennker 2011-12-13 03:56:05 PST
Created attachment 581222 [details]
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 2011-12-13 04:06:32 PST
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] 2011-12-14 07:12:08 PST
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) 2011-12-14 09:58:17 PST
(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.)

Note You need to log in before you can comment on or make changes to this bug.