Last Comment Bug 369157 - [FIX]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 soft...
Status: RESOLVED FIXED
: fixed1.8.0.10, fixed1.8.1.2, regression
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9alpha1
Assigned To: Boris Zbarsky [:bz]
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-02-02 16:31 PST by Tony Chung [:tchung]
Modified: 2007-03-02 00:00 PST (History)
12 users (show)
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
This fixes things over here (1007 bytes, patch)
2007-02-03 23:34 PST, Boris Zbarsky [:bz]
brendan: review+
brendan: superreview+
Details | Diff | Splinter Review
Fix one other caller, add JS_IsExceptionPending (2.23 KB, patch)
2007-02-04 11:18 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
With all comments addressed (2.12 KB, patch)
2007-02-04 13:58 PST, Boris Zbarsky [:bz]
dveditz: approval1.8.1.2+
dveditz: approval1.8.0.10+
Details | Diff | Splinter Review
Testcase (failing, even with latest trunk) (1.68 KB, patch)
2007-02-04 20:33 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review

Description Tony Chung [:tchung] 2007-02-02 16:31:14 PST
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] 2007-02-02 16:36:55 PST
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 [:rstrong] (use needinfo to contact me) 2007-02-02 16:43:30 PST
This may very well be a bug with the amo script for installing themes and not a xpinstall bug.
Comment 3 Mike Shaver (:shaver -- probably not reading bugmail closely) 2007-02-02 17:08:36 PST
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) 2007-02-02 17:21:05 PST
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) 2007-02-02 18:47:15 PST
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 [:rstrong] (use needinfo to contact me) 2007-02-02 20:08:50 PST
(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) 2007-02-02 21:09:36 PST
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) 2007-02-03 00:16:04 PST
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] 2007-02-03 11:00:34 PST
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 [:rstrong] (use needinfo to contact me) 2007-02-03 15:44:21 PST
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) 2007-02-03 22:38:22 PST
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 [:bz] 2007-02-03 23:13:28 PST
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 [:bz] 2007-02-03 23:34:33 PST
Created attachment 253929 [details] [diff] [review]
This fixes things over here
Comment 14 Brendan Eich [:brendan] 2007-02-04 11:09:14 PST
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 [:bz] 2007-02-04 11:18:16 PST
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...
Comment 16 Boris Zbarsky [:bz] 2007-02-04 11:19:32 PST
Oh, and shaver suggested the JS_IsExceptionPending checks.
Comment 17 Brendan Eich [:brendan] 2007-02-04 11:35:45 PST
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 [:bz] 2007-02-04 11:40:57 PST
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] 2007-02-04 13:35:56 PST
(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 [:bz] 2007-02-04 13:58:30 PST
Created attachment 253973 [details] [diff] [review]
With all comments addressed
Comment 21 Boris Zbarsky [:bz] 2007-02-04 14:07:43 PST
Fixed on trunk.
Comment 22 Daniel Veditz [:dveditz] 2007-02-04 15:09:24 PST
Comment on attachment 253973 [details] [diff] [review]
With all comments addressed

a=dveditz for 1.8/1.8.1 branches
Comment 23 Boris Zbarsky [:bz] 2007-02-04 19:35:44 PST
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 [:bz] 2007-02-04 20:22:22 PST
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] (remove +bmo to email) 2007-02-04 20:33:25 PST
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 26 Jeff Walden [:Waldo] (remove +bmo to email) 2007-02-04 21:12:14 PST
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 [:bz] 2007-03-02 00:00:43 PST
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.

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