The default bug view has changed. See this FAQ.

[FIX]Clicking install themes button will toggle between save to disk and software installation window

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
DOM: Events
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: tchung, Assigned: bz)

Tracking

({fixed1.8.0.10, fixed1.8.1.2, regression})

Trunk
mozilla1.9alpha1
fixed1.8.0.10, fixed1.8.1.2, regression
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

10 years ago
Repro:
1) FF 2.001 - Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a2pre) Gecko/20070129 Minefield/3.0a2pre
2) Mac or Windows OS
3) go to AMO
4) select themes
5) Pick any theme (https://addons.mozilla.org/firefox/4320/)
6) CLick the "install now" button.  The software installation window for extensions pops up.  Click "Cancel."
7) Click the "Install now" button again.  Now the "Save to Disk" for .jar file pops up.  Click "Cancel" again
8) Verify steps 6 and 7 toggle back and forth.

Expected:
- Install now should always pop up the Software Installation window

Actual:
- Toggles the window between software installation and save to disk

** Note, this doenst repro with addons and extensions.
The second click (that activates the download dialog) produces the following error:

Error: uncaught exception: Security Error: Content at https://addons.mozilla.org/firefox/4320/ may not load data from http://releases.mozilla.org/pub/mozilla.org/themes/patriotfox/patriotfox-1.0.0-fx.jar.
Assignee: nobody → xpi-engine
Component: Extension/Theme Manager → Installer: XPInstall Engine
Product: Firefox → Core
QA Contact: extension.manager
Target Milestone: Firefox 2 → ---
Version: 2.0 Branch → 1.8 Branch
This may very well be a bug with the amo script for installing themes and not a xpinstall bug.
Robert: under what circumstances _should_ that error be produced for an installTrigger call?  I can't figure out how even a buggy installTrigger call -- we run the same code each time you click, AFAICT -- would cause that security error to be thrown other than via a bug in xpinstall, but I could certainly be missing something.

I think there's another bug on this somewhere, too, with exactly that security error in play...
STR from Necko's perspective:

1. Someone wants us to load a cacheable stream with "Content-Type: application/x-java-archive", load and cache it for them.
2. /* Stupid user, staring at a dialog watching a countdown */
3. Get a request for an application/x-java-archive that's in the cache, serve it up

Should themes really be served as application/x-java-archive?
Actually, comment 4 is probably a red herring. However, riddle me this: why does http://dev.philringnalda.com/theme/ (which is view source, copy, paste, add <base href="https://addons.mozilla.org/">) work the second time, when AMO doesn't?
(In reply to comment #3)
> Robert: under what circumstances _should_ that error be produced for an
> installTrigger call?  I can't figure out how even a buggy installTrigger call
> -- we run the same code each time you click, AFAICT -- would cause that
> security error to be thrown other than via a bug in xpinstall, but I could
> certainly be missing something.
> 
> I think there's another bug on this somewhere, too, with exactly that security
> error in play...
There have been amo bugs previously due to the use of xmlhttprequest in the script that called installtrigger where the second click would fail. It is entirely possible this time it isn't due to the script but the previous 3+ times it was due to the amo script and reported to the incorrect component.

If it's AMO's fault, it's due to a correctness fix making them broken, because this regressed on the trunk some time in May (when all my old builds seem to be totally broken), and on the 1.8 branch during the pre-Fx2.0b1 landing rush, between 2006-07-01 15:00 and 2006-07-07 23:00. Many juicy possibilities in there, including the JS 1.7 landing, and at least two vaguely interesting security bugs which are still closed, bug 340107 and bug 337462.

If someone with a better supply of builds, or the bandwidth to download a week or month's worth, could say what day in July Fx2 stopped working, and when in May the trunk stopped working, that ought to narrow it down nicely.
Keywords: regression
The regression range for this is 2006-06-03:13 - 2006-06-03:22:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-06-03+12%3A00&maxdate=2006-06-03+23%3A00 
It's not the XPInstall engine. There's a toolkit "xpinstall" chrome that's touched in that regression window, for bug 245737 (checked in by Gavin), but that's just getting the countdown delay from a pref.

I tried to run js debugger on the page and put a breakpoint in the page's installTheme() routine (in the install.js file). When I hit the breakpoint the routine ran and called InstallTrigger.installChrome() fine. The other times it went straight to the download file and the onclick handler wasn't even called.

How is the event getting to the link, but sometimes bypassing the onclick handler?
Assignee: xpi-engine → events
Component: Installer: XPInstall Engine → DOM: Events
QA Contact: ian
not sure if it is relevant but here is one of the bugs where trying to install a theme a second time would fail - Bug 295366
Double-checking the branch with official Windows builds instead of my own Mac builds, the branch got it between the 2006-07-06:03 and 2006-07-07:04 builds -

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-07-06+02%3A00&maxdate=2006-07-07+05%3A00&cvsroot=%2Fcvsroot

The only things I see in common with the trunk window are that some of the things that were checked in for bug 326466 were also in the JS 1.7 branch-landing.

I'm not sure what to make of the fact that I can't reproduce it in https://landfill.bugzilla.org/bugzilla-tip/attachment.cgi?id=589 which, other than images and HTTP headers, is a clone of the AMO page. Hard to testcase something that refuses to show up on any other server.
Yeah, nothing in the trunk regression range other than the JS changes should really have affected this sort of thing...

So I debugged a tad, and here's what's going on.  That error comes from the _first_ ("successful") attempt to install.  To be precise, that attempt to install ends up doing an XMLHttpRequest for the URL "https://addons.mozilla.org/install.php?uri=http://releases.mozilla.org/pub/mozilla.org/themes/patriotfox/patriotfox-1.0.0-fx.jar".  The server sends back a 302 to "http://releases.mozilla.org/pub/mozilla.org/themes/patriotfox/patriotfox-1.0.0-fx.jar".  But the XMLHttpRequest was started from https://addons.mozilla.org/firefox/4320/, so can't get data from http://releases.mozilla.org/.  nsXMLHttpRequest::OnChannelRedirect performs a check via nsScriptSecurityManager::CheckSameOrigin, which, when the same-origin check fails, calls JS_SetPendingException on the passed-in context.

And then nothing reports the exception, I bet.  Which means the next time script tries to execute on that context (if you click again, e.g.), you get the exception reported and the onclick handler in question throws.  So then you're not actually running installTheme, but just loading the jar.
Flags: blocking1.9?
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.10?
Created attachment 253929 [details] [diff] [review]
This fixes things over here
Attachment #253929 - Flags: superreview?(brendan)
Attachment #253929 - Flags: review?(brendan)
Comment on attachment 253929 [details] [diff] [review]
This fixes things over here

r+sr=me -- any more places like this one?

/be
Attachment #253929 - Flags: superreview?(brendan)
Attachment #253929 - Flags: superreview+
Attachment #253929 - Flags: review?(brendan)
Attachment #253929 - Flags: review+
Created attachment 253957 [details] [diff] [review]
Fix one other caller, add JS_IsExceptionPending

Yeah, we have one other problematic caller.  All others either are called from script directly or rely on what's already on the JSContext stack...
Assignee: events → bzbarsky
Attachment #253929 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #253957 - Flags: superreview?(brendan)
Attachment #253957 - Flags: review?(brendan)
Attachment #253957 - Flags: approval1.8.1.2?
Attachment #253957 - Flags: approval1.8.0.10?
Oh, and shaver suggested the JS_IsExceptionPending checks.
Summary: Clicking install themes button will toggle between save to disk and software installation window → [FIX]Clicking install themes button will toggle between save to disk and software installation window
Target Milestone: --- → mozilla1.9alpha
Version: 1.8 Branch → Trunk
Comment on attachment 253957 [details] [diff] [review]
Fix one other caller, add JS_IsExceptionPending

No, we should not bloat the caller with JS_IsPendingException(cx) tests -- JS_ReportPendingException(cx) returns true if nothing is pending.

/be
But it does various work before that, no?

In any case; I'm happy to remove those checks before checkin.
(In reply to comment #18)
> But it does various work before that, no?

Nothing important to optimize away, and nothing with side effects.
 
> In any case; I'm happy to remove those checks before checkin.

My r+sr carries over with the removal of JS_IsPendingException tests.

/be
Created attachment 253973 [details] [diff] [review]
With all comments addressed
Attachment #253957 - Attachment is obsolete: true
Attachment #253973 - Flags: approval1.8.1.2?
Attachment #253973 - Flags: approval1.8.0.10?
Attachment #253957 - Flags: superreview?(brendan)
Attachment #253957 - Flags: review?(brendan)
Attachment #253957 - Flags: approval1.8.1.2?
Attachment #253957 - Flags: approval1.8.0.10?
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Comment on attachment 253973 [details] [diff] [review]
With all comments addressed

a=dveditz for 1.8/1.8.1 branches
Attachment #253973 - Flags: approval1.8.1.2?
Attachment #253973 - Flags: approval1.8.1.2+
Attachment #253973 - Flags: approval1.8.0.10?
Attachment #253973 - Flags: approval1.8.0.10+
Fixed on branches.

Not quite sure what a decent way of writing a test would be; it'd need server support no matter what.
Flags: blocking1.9?
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.10?
Keywords: fixed1.8.0.10, fixed1.8.1.2
Although... if this is a sync request, we may not want to report the error here.  What do you think?
Created attachment 254001 [details] [diff] [review]
Testcase (failing, even with latest trunk)

I think this testcase is correct, but it's failing right now -- the onload handler is getting called, but www.example.com is a different domain from localhost -- isn't it?
Comment on attachment 254001 [details] [diff] [review]
Testcase (failing, even with latest trunk)

Er.  So I've been reminded the test runs with full privileges and wouldn't be stopped.  The HTTP server will need CGI-like capabilities to handle this...
Attachment #254001 - Attachment is obsolete: true
On second thought, since we'd throw a separate exception from Send() anyway in this case, we should really be reporting this one.  Makes it uncatchable even for a sync XMLHttpRequest, but oh well.
You need to log in before you can comment on or make changes to this bug.