Addons deployed on github stopped updating

RESOLVED FIXED in Firefox 26, Firefox OS v1.2

Status

()

Core
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: yury, Assigned: Deian Stefan)

Tracking

(Blocks: 1 bug, {regression, verifyme})

26 Branch
mozilla27
regression, verifyme
Points:
---

Firefox Tracking Flags

(firefox25 unaffected, firefox26+ verified, firefox27+ verified, b2g-v1.2 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
STR:
1. Install addon from http://yurydelendik.github.io/shumway/extension/firefox/shumway.xpi
2. Notice, in the Add-ons Manager / Extensions, Shumway has version 0.7.185
3. Using tools for add-ons ("gear" icon), check for updates

Expected:
Shumway has version 0.7.280 or higher

Actual:
The version did not changed


Last good nightly: 2013-09-12
First bad nightly: 2013-09-13

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a4e9c9c9dbf9&tochange=b9029b1de410

Suspects (found using mozilla-inbound builds):
	1a475fdee12b	Sid Stamm — bug 836922 - (CSP) remove intersectWith once multiple policies are supported. r=grobinson
	5e4b386dc081	Sid Stamm — bug 836922 - tests for report-only and regular policies at the same time. r=grobinson
	1d23736e3779	Sid Stamm — bug 836922 - support mulitiple CSP policies at the same time. r=jst,grobinson


Originally reported at https://github.com/mozilla/pdf.js/issues/3719
Flags: needinfo?(sstamm)
CCing cviecco since I saw some PKIX validation problems in the debug stream while I was trying to reproduce.
This is what I get in a debug build when I attempt to update:

WARNING: NS_ENSURE_TRUE(!(aRv.Failed())) failed: file /home/sstamm/src/mozilla-central/content/base/src/nsXMLHttpRequest.cpp, line 3658
(pkix_CacheCert_Add: PKIX_PL_HashTable_Add for Certs skipped: entry existed
(pkix_CacheCert_Add: PKIX_PL_HashTable_Add for Certs skipped: entry existed

And the error console shows:
 WARN addons.updates: HTTP Request failed for an unknown reason
Flags: needinfo?(sstamm)
PKIX thing was a red herring.

bug 886164 changed the reference to the channel held by CSP to make it a weak reference.  CSPUtils.jsm:258 uses the docRequest to find the inner window (for error reporting). For some reason the updates fail unless we remove that line from CSPRep.fromString().

Here are the relevant warnings spit out when trying the update:

WARNING: NS_ENSURE_TRUE(mCacheEntry) failed: file /home/sstamm/src/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp, line 3607
CSP debug: CSP CREATED
CSP debug: CSP object initialized, no policies to enforce yet
CSP debug: APPENDING POLICY: default-src *; script-src 'self' https://github.global.ssl.fastly.net https://jobs.github.com https://ssl.google-analytics.com https://collector.githubapp.com https://analytics.githubapp.com; style-src 'self' 'unsafe-inline' https://github.global.ssl.fastly.net; object-src 'self' https://github.global.ssl.fastly.net
CSP debug:             SELF: https://raw.github.com/mozilla/shumway/gh-pages/extension/firefox/update.rdf
CSP debug: CSP 1.0 COMPLIANT : false
WARNING: NS_ENSURE_TRUE(!(aRv.Failed())) failed: file /home/sstamm/src/mozilla-central/content/base/src/nsXMLHttpRequest.cpp, line 3658
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /home/sstamm/src/mozilla-central/content/base/src/nsDocument.cpp, line 2473
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /home/sstamm/src/mozilla-central/content/base/src/nsDocument.cpp, line 2643
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /home/sstamm/src/mozilla-central/content/base/src/nsDocument.cpp, line 2421
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /home/sstamm/src/mozilla-central/content/base/src/nsXMLHttpRequest.cpp, line 2046

I don't understand quite enough about XPConnect to figure out what's really going on here since I can't catch the failure from inside the function that returns a failure.  Apparently nsIContentSecurityPolicy::AppendPolicy() is returning NS_ERROR_FAILURE, but when I wrap the entire body of the implementation in a try/catch, there's no exceptions thrown to indicate this.

bholley, deian: can you take a look and give me a little advice?
Blocks: 493857
Depends on: 886164
Flags: needinfo?(deian)
Flags: needinfo?(bobbyholley+bmo)
So, the problem is that someone tries to getInterface on an XHR, which fails. That alone is fine, but our bindings code borks the whole thing by returning false without setting an exception on the cx, which turns this failure into an uncatchable OOM that goes postal on the entire callstack.

The issue is here:

http://mxr.mozilla.org/mozilla-central/source/dom/bindings/Exceptions.cpp#117

Kyle, what's the intent here? Shouldn't we be re-reporting the exception onto the cx?
Flags: needinfo?(khuey)
Flags: needinfo?(deian)
Flags: needinfo?(bobbyholley+bmo)
Yeah that's wrong.
Flags: needinfo?(khuey)
tracking-firefox26: --- → ?
tracking-firefox27: --- → ?
status-firefox25: --- → unaffected
status-firefox26: --- → affected
status-firefox27: --- → affected
tracking-firefox26: ? → +
tracking-firefox27: ? → +
needinfo on Sid here to help with assignee.
Flags: needinfo?(sstamm)
Redirecting for a little hlep.  Admittedly, I don't understand our bindings very well.  bholley: you helped me debug this... do you know who can fix this?
Flags: needinfo?(sstamm) → needinfo?(bobbyholley+bmo)
(Assignee)

Comment 8

5 years ago
Created attachment 815113 [details] [diff] [review]
0001-Bug-919209-Throw-re-reports-exception-onto-context.patch
Attachment #815113 - Flags: review?(bobbyholley+bmo)
(Assignee)

Updated

5 years ago
No longer depends on: 886164
Attachment #815113 - Flags: review?(bobbyholley+bmo) → review+
(Assignee)

Comment 9

5 years ago
Try looks good:
https://tbpl.mozilla.org/?tree=Try&rev=17c73b058e7d
Flags: needinfo?(bobbyholley+bmo)
(Assignee)

Comment 10

5 years ago
I think this is ready for checkin.
(Reporter)

Updated

5 years ago
Assignee: nobody → deian
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7f9c148d281c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
can we get a beta uplift nomination for this regression with a risk/reward assessment?  it's still early in beta so we can take a low-risk uplift.
status-firefox27: affected → fixed
Flags: needinfo?(deian)

Updated

5 years ago
Keywords: verifyme
(Assignee)

Updated

5 years ago
Flags: needinfo?(deian)
(Assignee)

Comment 15

5 years ago
Comment on attachment 815113 [details] [diff] [review]
0001-Bug-919209-Throw-re-reports-exception-onto-context.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: breaks certain add-ons (probably worse)
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): low, patch propagates as opposed to failing hard
String or IDL/UUID changes made by this patch: none
Attachment #815113 - Flags: approval-mozilla-beta?
Yeah, if this has baked on m-c and m-a it's probably ok, but we should make sure that the relevant DOM bindings machinery (in particular ThrowExceptionObject) doesn't do something different on beta.
Comment on attachment 815113 [details] [diff] [review]
0001-Bug-919209-Throw-re-reports-exception-onto-context.patch

Approving for beta uplift, we'll have one more beta next week so if there is any unusual results from this, we can back out then.
Attachment #815113 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-b2g-v1.2: --- → fixed
Verified as fixed on FF 26.08 on the following environments:

Windows 7 x64
Ubuntu 12.04 x32
OS x 10.8.5

The shumway add-on from github is successfully updated.
status-firefox26: fixed → verified
Verified as fixed with Firefox 27 beta 1 (build ID: 20131209204824) on: Win 7 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.7.5
status-firefox27: fixed → verified
You need to log in before you can comment on or make changes to this bug.