Closed Bug 295366 Opened 19 years ago Closed 19 years ago

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

Categories

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

Product:
addons.mozilla.org Graveyard
Component:
Administration
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: tracy, Assigned: cso)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

testcase
1.65 KB, text/html
Details
patch
2.05 KB, patch
morgamic
: first-review+
Details | Diff | Splinter Review 
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.
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.
Flags: blocking1.8b3?
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...
Flags: blocking1.8b3?
Flags: blocking-aviary1.1?
Reproduced using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050524 Firefox/1.0+ ID:2005052405
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..
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.
Severity: blocker → major
Keywords: smoketest
OS: Windows XP → All
Hardware: PC → All
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.

This has been around a while.  The oldest available nightly (without going to
archive), 2005-05-09-07-trunk has this bug.
Summary: Themes not installing automatically → Themes won't installing automatically after "cancel" to confirmation dialog
Does this only happen on UMO? Are there any JS Console errors with the
showInConsole pref set?
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+
Ah, that would indicate that this is a UMO specific problem. Can anyone else
reproduce somewhere other than UMO?
(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.
Tracy, is this still a problem with recent trunk builds?
yes, still a problem on all platforms when cancelling and retrying at UMO. Seeing the same error in js 
console as comment #9
Summary: Themes won't installing automatically after "cancel" to confirmation dialog → Themes won't install automatically after "cancel" to confirmation dialog
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?
The issue is with the use of nsIXMLHttpRequest on UMO... changing product, etc.
Component: Extension/Theme Manager → Administration
Flags: blocking-aviary1.1?
Product: Firefox → Update
Version: Trunk → 1.0
Assignee: nobody → Bugzilla-alanjstrBugs
QA Contact: extension.manager → administration
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?
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.
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.
The mirrors have the proper mimetype for .xpi.  Themes are .jar.
(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
Attached file 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.
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.
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.
(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...
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.
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.
Attached patch UMO Workaround (obsolete) — — Splinter Review 
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.
Attachment #186517 - Flags: first-review?(cst)
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;
}

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.
(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).
Attachment #186517 - Flags: first-review?(cst) → first-review?(mike.morgan)
Oh, its a workaround to that.  In that case, the try-catch is appropriate, but
asynchronously, please.
Depends on: 306643
Colin's patch.
Assignee: Bugzilla-alanjstrBugs → colin.ogilvie
I'm fairly sure this patch should be r-'ed because the XMLHttpRequest needs to
be async.
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;
}
Attached patch patch — — Splinter Review 
Don't know who would be best to review this, etc.
Attachment #197385 - Flags: first-review?(morgamic)
Comment on attachment 186517 [details] [diff] [review]
UMO Workaround

Patch is made obselete by the latest patch.
Attachment #186517 - Attachment is obsolete: true
Attachment #186517 - Flags: first-review?(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.
Attachment #197385 - Flags: first-review?(morgamic) → first-review+
Just tested theme installs on AMO and the patch has fixed this bug.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Tested with Windows and Linux Firefox mozilla1.8 branch builds 0930.  Theme
installation works as expected even after cancelling and retrying.

verified
Status: RESOLVED → VERIFIED
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: