Last Comment Bug 725016 - Need to restart firefox 12.0a2 twice to get specific extension to work
: Need to restart firefox 12.0a2 twice to get specific extension to work
Status: RESOLVED FIXED
[qa+]
: addon-compat, regression
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: 12 Branch
: x86_64 All
: -- normal with 1 vote (vote)
: mozilla12
Assigned To: Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
:
Mentors:
Depends on:
Blocks: 675221
  Show dependency treegraph
 
Reported: 2012-02-07 10:57 PST by Cyril
Modified: 2012-05-15 15:06 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified


Attachments
Extension with reordered chrome.manifest file : category command at the end (43.71 KB, application/octet-stream)
2012-02-07 22:26 PST, Cyril
no flags Details
Patch, make category delivery always be asynchronous, rev. 1 (1.21 KB, patch)
2012-02-08 11:06 PST, Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Test (2.83 KB, patch)
2012-02-08 11:06 PST, Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
no flags Details | Diff | Splinter Review

Description Cyril 2012-02-07 10:57:00 PST
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20100101 Firefox/10.0
Build ID: 20120205184700

Steps to reproduce:

Install Firefox 12.0a2 and create fresh profile.
Install extension at
https://addons.mozilla.org/fr/firefox/addon/html-notifications/
and restart.
Go to http://code.google.com/p/ff-html5notifications/ and test notifications.


Actual results:

Firefox says functionnality is not available.


Expected results:

Firefox should have asked for notification permission.

After a second restart, Firefox does ask for notification permission.

This extension uses no binary code, but (among others) Javascript XPCOM and javascript global property with chrome.manifest registration
category JavaScript-global-property webkitNotifications @paxal.net/html5notifications/notification-center;1

Addons Log after first restart :
*** LOG addons.xpi: startup
*** LOG addons.xpi: checkForChanges
*** LOG addons.xpi: Found updated metadata for html5notifications@paxal.net in app-profile
*** LOG addons.xpi: Processing install of html5notifications@paxal.net in app-profile
*** LOG addons.xpi: Opening database
*** LOG addons.xpi: New add-on html5notifications@paxal.net installed in app-profile
*** LOG addons.xpi: Updating database with changes to installed add-ons
*** LOG addons.xpi: Updating add-on states
*** LOG addons.xpi: Writing add-ons list

See issue @ http://code.google.com/p/ff-html5notifications/issues/detail?id=36
Comment 1 Alice0775 White 2012-02-07 11:49:47 PST
Regression window
Cannot reproduce:
http://hg.mozilla.org/mozilla-central/rev/4de07a341aab
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120111 Firefox/12.0a1 ID:20120111082226
Can reproduce:
http://hg.mozilla.org/mozilla-central/rev/17a76e33b6fe
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120111 Firefox/12.0a1 ID:20120111083049
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4de07a341aab&tochange=17a76e33b6fe

Suspected: Bug 675221
Comment 2 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-02-07 12:55:08 PST
This extension needs to be fixed. What's happening is:

We hit the line "category JavaScript-global-property webkitNotifications @paxal.net/html5notifications/notification-center;1" in chrome.manifest. When we process this line, before the patch we would post an event to the event loop to notify observers. The new behavior is that we immediately notify observers. Both behaviors are correct, but the new behavior is much more efficient.

This ends up immediately at http://hg.mozilla.org/mozilla-central/annotate/e0d9c8ddd5bd/dom/base/nsScriptNameSpaceManager.cpp#l724 which fails because the contractID isn't registered yet.

This would be solved by reordering chrome.manifest so that you register the contractid/CID *before* you add the category.

It's not clear to me why this works correctly on non-first runs, but that actually sounds like a bug that we need to fix; the behavior should be consistent.
Comment 3 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-02-07 13:01:44 PST
The reason this doesn't happen on second-startup is that we initialize the nsScriptNameSpaceManager later in startup, so that it doesn't have to cache the results.
Comment 4 Cyril 2012-02-07 22:24:00 PST
Hi,

Thank you for such quick replies.

I reordered chrome.manifest lines, but the problem is still here.
The components are accessible, but the JavaScript global-property is not.

Extract of chrome.manifest :
# notification center object
interfaces components/nsIPXLNotificationCenter.xpt
component {d931339b-60b1-4559-9459-a69ed1396561} components/nsIPXLNotificationCenter.js
contract @paxal.net/html5notifications/notification-center;1 {d931339b-60b1-4559-9459-a69ed1396561}
category JavaScript-global-property webkitNotifications @paxal.net/html5notifications/notification-center;1

I will ask other AMO Editors if they can find other addons that could be concerned with this issue. Maybe it's a problem with mine only.
Comment 5 Cyril 2012-02-07 22:26:00 PST
Created attachment 595321 [details]
Extension with reordered chrome.manifest file : category command at the end
Comment 6 Cyril 2012-02-08 04:58:56 PST
I tested 3 (mine + 2) of the 12 addons with JavaScript-global-property in chrome.manifest (thank you AMO editors for the list) :

https://addons.mozilla.org/fr/firefox/addon/js-print-setup/ (11.0b1 OK, 12.0a2 NOT OK)
Javascript property : jsPrintSetup
chrome.manifest : ordered

https://addons.mozilla.org/fr/firefox/addon/openid-for-firefox/ (11.0b1 OK, 12.0a2 NOT OK)
Javascript property : openid
chrome.manifest : ordered

All addons with Global JS Property stop working after 1st restart, and work again after second restart (even the addons that were already installed).

Small interface to test if a window.javascriptProperty exists :
http://paxal.net/bmo725016/
Comment 7 Nils Maier [:nmaier] 2012-02-08 05:03:42 PST
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> This would be solved by reordering chrome.manifest so that you register the
> contractid/CID *before* you add the category.

Reordering will not have any effect as contracts are special cased in the manifest parser to be registered last:
http://mxr.mozilla.org/mozilla-central/source/xpcom/components/ManifestParser.cpp#511
Comment 8 Nils Maier [:nmaier] 2012-02-08 08:14:32 PST
A viable work-around would be to register components in a separate manifest and link that from the main manifest, as the Parser will only delay contract registration on a per manifest basis.

E.g.
chrome.manifest:
manifest components.manifest
category ...

components.manifest:
interfaces ...
component ...
contract ...

Right now nsScriptNameSpaceManager.cpp - the global javascript components - and the nsIStringBundleService (checking for nsIStringBundleOverride.idl) are the only code bits in core code that observe "xpcom-category-entry-added".

The only addons mxr listed add-on observing the notification is AdBlock Plus, using this as a way for other extensions to load modules conditionally on ABP being installed:
https://mxr.mozilla.org/addons/source/1865/modules/Bootstrap.jsm#127
ABP hg is down ATM, at least for me, so here are the commits from a clone:
https://bitbucket.org/ericpaulbishop/trueblockplus/changeset/bc30fc47654a
https://bitbucket.org/ericpaulbishop/trueblockplus/changeset/5dfabe432ddb
There are no addons mxr listed add-ons registering themselves to the "adblock-plus-module-location" category.

If this bug does not get fixed, this definitively needs a note regarding js globals, stringbundle overrides and the xpcom-category-entry-added notification in the compatibility report Jorge publishes on the addons blog.
Comment 9 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-02-08 08:22:03 PST
Hrmph. I guess we should always asynchronously dispatch this notification. Patch forthcoming :-(
Comment 10 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-02-08 11:06:05 PST
Created attachment 595456 [details] [diff] [review]
Patch, make category delivery always be asynchronous, rev. 1
Comment 11 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-02-08 11:06:25 PST
Created attachment 595457 [details] [diff] [review]
Test
Comment 12 Boris Zbarsky [:bz] 2012-02-08 13:11:32 PST
Comment on attachment 595456 [details] [diff] [review]
Patch, make category delivery always be asynchronous, rev. 1

r=me
Comment 13 Wong Huong Cheong 2012-02-09 00:32:32 PST
I have same problem, but can't solve by restart twice
Comment 14 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-02-09 08:38:55 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/085725c8d753
https://hg.mozilla.org/integration/mozilla-inbound/rev/e550d86e3ee9

I totally screwed up the bug number in parts of this :-(
Comment 16 Wong Huong Cheong 2012-02-10 06:42:47 PST
Target Milestone set to mozilla13, is that mean only in firefox 13?
so, firefox 12a2 won't enjoy the fix, right?
Comment 17 Jorge Villalobos [:jorgev] 2012-02-13 07:04:53 PST
*** Bug 696117 has been marked as a duplicate of this bug. ***
Comment 18 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-02-13 08:09:24 PST
Comment on attachment 595456 [details] [diff] [review]
Patch, make category delivery always be asynchronous, rev. 1

This is pretty low-risk, it reverts this specific case to the previous behavior and comes with a test.
Comment 19 Alex Keybl [:akeybl] 2012-02-13 10:44:19 PST
Comment on attachment 595456 [details] [diff] [review]
Patch, make category delivery always be asynchronous, rev. 1

[Triage Comment]
This low-risk patch fixes a reproducible bug when an add-on needs notification permissions. Approved for Aurora 12.
Comment 20 Alex Keybl [:akeybl] 2012-03-02 15:02:54 PST
This hasn't yet landed on Aurora 12 - can we do that soon so that this doesn't land right before the cutover to beta?
Comment 22 Virgil Dicu [:virgil] [QA] 2012-03-30 08:44:41 PDT
Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0) Gecko/20100101 Firefox/12.0

Verified with steps from comment 0 on Ubuntu 11.10 x86_64 and Mac OS 10.6. Notifications appear after restarting.

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