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

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Tony Chung [:tchung]]

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.

Comment 1

[Dave Townsend [:mossop]]

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.

Comment 2

[Robert Strong (they/them - no direct email)]

This may very well be a bug with the amo script for installing themes and not a xpinstall bug.

Comment 3

[Mike Shaver (:shaver emeritus)]

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...

Comment 4

[Phil Ringnalda (:philor)]

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?

Comment 5

[Phil Ringnalda (:philor)]

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?

Comment 6

[Robert Strong (they/them - no direct email)]

(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.

Comment 7

[Phil Ringnalda (:philor)]

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.

Comment 8

[Ria Klaassen (not reading all bugmail)]

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 

Comment 9

[Daniel Veditz [:dveditz]]

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?

Comment 10

[Robert Strong (they/them - no direct email)]

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

Comment 11

[Phil Ringnalda (:philor)]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: This fixes things over here

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 253929 [details] [diff] [review]
This fixes things over here

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

/be

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: 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...

Comment 16

[Boris Zbarsky [:bzbarsky]]

Oh, and shaver suggested the JS_IsExceptionPending checks.

Comment 17

[Brendan Eich [:brendan]]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

But it does various work before that, no?

In any case; I'm happy to remove those checks before checkin.

Comment 19

[Brendan Eich [:brendan]]

(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

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: With all comments addressed

Comment 21

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 253973 [details] [diff] [review]
With all comments addressed

a=dveditz for 1.8/1.8.1 branches

Comment 23

[Boris Zbarsky [:bzbarsky]]

Fixed on branches.

Not quite sure what a decent way of writing a test would be; it'd need server support no matter what.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Although... if this is a sync request, we may not want to report the error here.  What do you think?

Comment 25

[Jeff Walden [:Waldo]]

Attachment: 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 26

[Jeff Walden [:Waldo]]

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...

Comment 27

[Boris Zbarsky [:bzbarsky]]

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.