295366 - Themes won't install automatically after "cancel" to confirmation dialog

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Administration, defect)

Description

[Tracy Walker [:tracy]]

Seen on Windows Deer Park build 2005-05-24-06-trunk

note: This regressed since the previous nightly of 2005-05-23-11-trunk on Windows

-Go to Tools | Themes
-In the Theme Manager, click "Get More Themes" link
-Choose a theme, then click the Install Now link

tested results:
A choose dialog appears asking to open with... or save to disc.
(work-around: save to disc then drag the saved file to the Theme Manager to
install the theme)

Expected results:
A notification message that the theme will be installed appears. Clicking okay
to that dialog installs the theme.

Comment 1

[Tracy Walker [:tracy]]

further testing has shown that sometimes theme installation works as expected. 
However, I have found an additional step that seems to make this reliably
reproducable.

-Click Install Now for the chosen theme
-If the expected confirm dialog appears, click the "Cancel" button.
-Then, click the Install Now link again

This will bring up the chooser instead of the confirm dialog everytime.

Comment 2

[Caleb]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050524
Firefox/1.0+ ID:2005052401

I can't reproduce this bug...

Comment 3

[Chris Blore]

Reproduced using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050524 Firefox/1.0+ ID:2005052405

Comment 4

[Caleb]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050524
Firefox/1.0+ ID:2005052411

I've tried it in safe-mode aswell.

Themes > Get More Themes > Qute > Install.

I get the theme installtion message box, I say Cancel, press Install again, it
shows the message box again, etc.. etc..

Comment 5

[Tracy Walker [:tracy]]

Hmmm, wonders if network issues at MoFo are contributing to this? But, I can not
reproduce it on Fx 1.0.4.

Using the cancel step I was able to reproduce this with yesterdays build.  I'm
in the middle of 1.1 BFT's and don't have time to track down if/when this
regressed.    

Also removing smoketest blocker status as attempting to install after "cancel"
isn't a smoketest. 

Comment 6

[Tristor]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050524
Firefox/1.0+

I still see this, on an hourly build from today.  After hitting "install now", I
am prompted with the install dialog, I hit cancel, hit the "install now" again,
and am prompted to save the file instead of to install.

Comment 7

[Tracy Walker [:tracy]]

This has been around a while.  The oldest available nightly (without going to
archive), 2005-05-09-07-trunk has this bug.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Does this only happen on UMO? Are there any JS Console errors with the
showInConsole pref set?

Comment 9

[Teune van Steeg]

I get this error in the JSConsole after trying to install the theme for the
second time:

Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIXMLHttpRequest.send]"  nsresult: "0x80004005
(NS_ERROR_FAILURE)"  location: "JS frame ::
https://addons.mozilla.org/themes/moreinfo.php?application=firefox&category=Modern&numpg=10&id=370
:: installTheme :: line 85"  data: no]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050523
Firefox/1.0+

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Ah, that would indicate that this is a UMO specific problem. Can anyone else
reproduce somewhere other than UMO?

Comment 11

[Tristor]

(In reply to comment #10)
> Ah, that would indicate that this is a UMO specific problem. Can anyone else
> reproduce somewhere other than UMO?

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050524
Firefox/1.0+

I tried several sites that had themes, with varying methods of installation from
using PHP to Javascript to do installs.  Was unable to reproduce the problem
outside of UMO.

Comment 12

[sairuh (rarely reading bugmail)]

Tracy, is this still a problem with recent trunk builds?

Comment 13

[Tracy Walker [:tracy]]

yes, still a problem on all platforms when cancelling and retrying at UMO. Seeing the same error in js 
console as comment #9

Comment 14

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

This seems to be specific to UMO's use of XMLHttpRequest. I tried the steps
using a simple InstallTrigger.installChrome without being able to reproduce
which you can verify here:
http://exchangecode.com/spellbound/silverskin.html

UMO uses the following javascript to install a theme and I believe something
similar for installing extensions:
function installTheme( aEvent, extName) {
var p = new XMLHttpRequest();
p.open("GET", "/core/install.php?uri="+aEvent.target.href, false);
p.send(null);
InstallTrigger.installChrome(InstallTrigger.SKIN,aEvent.target.href,extName);
return false;
}

It is failing with the XMLHttpRequest as shown in the js console which is not
part of the Extension/Theme Manager:
Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIXMLHttpRequest.send]"  nsresult: "0x80004005
(NS_ERROR_FAILURE)"  location: "JS frame ::
https://addons.mozilla.org/themes/moreinfo.php?application=firefox&category=Modern&numpg=10&id=370
:: installTheme :: line 85"  data: no]

Perhaps there is a bug with nsIXMLHttpRequest.send?

Comment 15

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

The issue is with the use of nsIXMLHttpRequest on UMO... changing product, etc.

Comment 16

[alanjstr]

I believe this javascript was meant to ensure that the file exists before we
trigger the install (bug 291357).  What would be the proper use of
XMLHttpRequest()?  And why would this only happen after you've cancelled?

Comment 17

[Colin Ogilvie [:cso]]

I can not reproduce this on

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.7.8) Gecko/20050511
Firefox/1.0.4

but can reproduce it on

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050613
Firefox/1.0+

This to me indicates that the problem is actually in Firefox, since I'm using
identical code on the update site.

Comment 18

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

Here is a link to a saved version of the silver skin page on UMO.
https://exchangecode.com/spellbound/test.html

It will require you to accept the cert for the session since it is from cacert.
I made one change to the function installTheme function as follows so the
request to verify that a file exists is to the same site as the page:
-p.open("GET", "/core/install.php?uri="+aEvent.target.href, false);
+p.open("GET", "https://exchangecode.com/spellbound/silverskin.html", false);

After clicking cancel to the first install I am unable to get it to fail on this
page using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050615 Firefox/1.0+ ID:2005061522 and I am able to get it to fail
consistently on UMO.

Why the XMLHttpRequest fails on the second click when viewing the page on UMO I
have no idea and like I said this may very well be a problem with
XMLHttpRequest... actually, it could be a problem with any number of the changes
that have occurred since 1.0. I am pretty sure that the Extensions/Themes
Manager or xpinstall code can be ruled out since the function on the page
doesn't even get to this code due to the exception thrown by XMLHttpRequest.
BTW: when the XMLHttpRequest exception is thrown it falls back to it just being
a click on the link which is why it always brings up the save dialog for themes
since none of the mirrors should have a mimetype of application/x-xpinstall. I
recall seeing a similar bug in regards to extensions and unless I'm mistaken not
all of the mirrors have the application/x-xpinstall mimetype for xpi files as
they should so this could also occur with extensions.

Comment 19

[alanjstr]

The mirrors have the proper mimetype for .xpi.  Themes are .jar.

Comment 20

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

(In reply to comment #19)
> The mirrors have the proper mimetype for .xpi.  Themes are .jar.
OK. There have just been times where I have been prompted to download vs.
install an .xpi when installing from some of the mirrors in the past and I
believe this same problem was once reported in relation to extensions though
that cleared itself up in a couple of days. Perhaps one of the mirrors didn't
have the proper mimetype for xpi's for a few days which would also explain why I
recall it wasn't consistently reproducible in that bug.

It appears that it is getting the data from the cache and it may have something
to do with the server not sending a response for the success case? I just threw
this together and it appears to WFM. There is no special handling of the case
when a file is not found as there wasn't previously. I just used responseText to
validate it and this responseText is also present in the cache even when it fails.
function installTheme(aEvent, extName) {
  var p = new XMLHttpRequest();
  p.open("GET",
"https://addons.mozilla.org/core/install.php?uri="+aEvent.target.href, false);
  try {
    p.send(null);
  } catch (e) { }
  if (p.responseText == "Invalid URI cannot Continue")
    return false;

  InstallTrigger.installChrome(InstallTrigger.SKIN,aEvent.target.href,extName);
  return false;
}

I would think the change in behavior would be due to one of these checkins but
which one I do not know:
http://webtools.mozilla.org/bonsai/cvslog.cgi?file=mozilla/extensions/xmlextras/base/src/nsXMLHttpRequest.cpp&rev=&root=/cvsroot

Comment 21

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

I am getting SSL Pass Thru errors and take a look at the difference in the
response between these two on the trunk.
http://addons.mozilla.org/core/install.php?uri=http://ftp.mozilla.org/pub/mozilla.org/themes/silver_skin/silver_skin-2.6.3-fx.jar

https://addons.mozilla.org/core/install.php?uri=http://ftp.mozilla.org/pub/mozilla.org/themes/silver_skin/silver_skin-2.6.3-fx.jar

Comment 22

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

Attachment: testcase

The testcase has two functions that use async XMLHttpRequest's - one with SSL
and the other without. You have to view it locally for it to work and you will
be prompted to grant the page UniversalBrowserRead priv's. It turns out that
the code doesn't receive the 301 responses when using SSL on the trunk...
you'll need to perform http logging to see this. With the non SSL sample it
does receive the 301 responses and it doesn't suffer from this bug.

Comment 23

[Colin Ogilvie [:cso]]

I'm still confused as to whether this is a Firefox bug or a UMO bug. Since it
works for me in 1.0.4 but not in Deer Park Alpha, to me it looks like Firefox.

However, is there something we (UMO) can do to work around this issue,
preferably without removing the XMLHttpRequest thing? If so, I think its
something we should look at.

Comment 24

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

I'm not sure whether it is intended behavior to not allow 301's via an
XMLHttpRequest over SSL where the 301 points to a page that is not using SSL but
that appears to be what is happening. By sending the XMLHttpRequest over a non
SSL connection it just works. Hopefully this is enough info to find out whether
or not this is intended behavior and then take it from there. I don't have a
clue as to the security ramifications of sending the XMLHttpRequest over a non
SSL connection but if there are none this appears to be one way to fix it on the
UMO side.

Comment 25

[Colin Ogilvie [:cso]]

(In reply to comment #24)
> I'm not sure whether it is intended behavior to not allow 301's via an
> XMLHttpRequest over SSL where the 301 points to a page that is not using SSL but
> that appears to be what is happening.

Erm, is the script thats being called sending a 301 header? It should only send
a 301 header for Thunderbird downloads (which has a passthrough=yes query-string
pair...

Comment 26

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

The "XMLHttpRequest without SSL" in the testcase is the one that I saw a 301
with and I recall getting the same response with 1.0.3. When using
"XMLHttpRequest with SSL" in the testcase I receive a SSL Pass Thru error the
same as I do on UMO which is what gave me the idea to create the first testcase
without SSL.

Comment 27

[Colin Ogilvie [:cso]]

Ah, in that case, you probably would see a 301 redirect - from http:// to
https:// - but I'm not sure how helpful that is in this case; there is no 301
redirect on the script on UMO from what I can see using LiveHTTPHeaders - it
gets a straight 200 response from the server.

Comment 28

[Colin Ogilvie [:cso]]

Attachment: UMO Workaround

Anyone:

Anyone that was seeing this issue on the live site, can you please check at
https://update-staging.mozilla.org/~colin/update/themes/moreinfo.php?application=firefox&id=7
(you'll need to accept the invalid certificate) and see if you no longer see
this problem.

Chris:

This works around the problems being seen on this bug, regardless of whether
its a UMO issue or a Firefox issue.

Comment 29

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

Colin - that works for me. As a side note it appears that doing this
asynchronously also works as shown in the testcase though I don't know if there
are any bad side affects.

function installThemeSSL(aEvent, extName) {
  var p = new XMLHttpRequest();
  p.open("GET", "/core/install.php?uri="+aEvent.target.href, true);
  p.onreadystatechange=function() {
    if (p.readyState == 4) {
      InstallTrigger.installChrome(InstallTrigger.SKIN,aEvent.target.href,extName);
      return false;
    }
  }
  p.send(null);

  return false;
}

Comment 30

[alanjstr]

If you try to call UMO itself with http, you'll get a redirect to the secure
site, yes.

The purpose of the XMLHttpRequest, as I said before, is to make sure the file
actually exists.  If we're not going to bother capturing the result, we
shouldn't bother with the call in the first place.  

Please address that issue on bug 291357.  This bug will probably go back to
nsIXMLHttpRequest nfor it mishandling 301 over SSL.

Comment 31

[Colin Ogilvie [:cso]]

(In reply to comment #30)
> The purpose of the XMLHttpRequest, as I said before, is to make sure the file
> actually exists.  If we're not going to bother capturing the result, we
> shouldn't bother with the call in the first place.  

No, the purpose of the XMLHttpRequest is to update the download counts for the
extension / theme - it does nothing about checking whether it exists and never
has done to the best of my knowledge. If you want it to start doing that, then
that is the other bug - but this is just a workaround to the problem being
highlighted by this bug which we CAN work around, while maintaining the code
exactly as it is (apart from the workaround, obviously).

Comment 32

[alanjstr]

Oh, its a workaround to that.  In that case, the try-catch is appropriate, but
asynchronously, please.

Comment 33

[alanjstr]

Colin's patch.

Comment 34

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

I'm fairly sure this patch should be r-'ed because the XMLHttpRequest needs to
be async.

Comment 35

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

Perhaps the following would be more appropriate. I suggest initiating the
install before initiating the aysnc XMLHttpRequest for tracking installs. This
also should be done for both extensions and themes... it isn't as noticeable
with extensions since most of the mirrors have the correct mimetype for xpi
files but every once in a while people are prompted to download the xpi instead
of the xpinstall dialog being displayed.

function installTheme( aEvent, extName) {
  InstallTrigger.installChrome(InstallTrigger.SKIN,aEvent.target.href,extName);

  try {
    var p = new XMLHttpRequest();
    p.open("GET", "/core/install.php?uri="+aEvent.target.href, true);
    p.send(null);
  }
  catch(e) { }
  return false;
}

function install( aEvent, extName, iconURL)  {   
  var params = new Array();
  params[extName] = {
    URL: aEvent.target.href,
    IconURL: iconURL,
    toString: function () { return this.URL; }
  };
  InstallTrigger.install(params);

  try {
    var p = new XMLHttpRequest();
    p.open("GET", "/core/install.php?uri="+aEvent.target.href, true);
    p.send(null);
  }
  catch(e) { }
  return false;
}

Comment 36

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

Attachment: patch

Don't know who would be best to review this, etc.

Comment 37

[Michael Morgan [:morgamic]]

Comment on attachment 186517 [details] [diff] [review]
UMO Workaround

Patch is made obselete by the latest patch.

Comment 38

[Michael Morgan [:morgamic]]

Comment on attachment 197385 [details] [diff] [review]
patch

Looks great.  Thanks for helping with that one.  Checked it in to
MOZILLA_UPDATE_1_0_BRANCH.

Comment 39

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

Just tested theme installs on AMO and the patch has fixed this bug.

Comment 40

[Tracy Walker [:tracy]]

Tested with Windows and Linux Firefox mozilla1.8 branch builds 0930.  Theme
installation works as expected even after cancelling and retrying.

verified