130428 - Triggering xpi with URL argument is not installing on WIN only

Status: VERIFIED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Jimmy Lee]

Build: 2002-03-12-10-trunk(WIN)

1. Open http://jimbob/jars/a_trigger_args.html
2. Click Trigger button
3. Click Install button from Software Installation dialog

RESULT:
Download dialog appears and disappears very quickly.  No installation takes
place.  Install.log shows nothing.  Mac and Linux behave fine.

EXPECTED RESULT:
Installs gracefully.

Comment 1

[Jimmy Lee]

Nominating for beta.  This needs to get fixed in order to be able to install
packages with URL parameters.  This is a problem on all Windows platforms.  This
used to work, and it is broken now.

Comment 2

[Jimmy Lee]

http://jimbob/jars/u_new65_arg.html is another test that fails, too.

Comment 3

[Syd Logan]

adt1 per adt triage

Comment 4

[Syd Logan]

prohibits installs from a cgi-bin script, reason this is adt1

Comment 5

[Syd Logan]

--> dprice

Comment 6

[dprice (gone)]

When I run this test on windows, I get an error "Not a valid install package"

The install log tells me it failed with error -207

Comment 7

[Jimmy Lee]

I get the same thing.  Also, http://jimbob/jars/u_new65_arg.html fails as well.
 I tried both Win 2000 and Win 98.  Something has gone wrong.  I can confirm
that an older browser does trigger as expected, so the test cases are working.

Comment 8

[dprice (gone)]

I am seeing two different problems while debugging this.

First I'm getting assertions in onStopRequest mItem->mOutputStream is null

Second GetScriptFromJarfile is failing.  It is passing the filename with the
query string still appended to nsJar::Open and no one in nsJar is bothering to
strip it away before trying to open the file

Comment 9

[Daniel Veditz [:dveditz] out until Nov 10]

nsJAR shouldn't have to worry about bogus filenames, XPInstall code should be
stripping that. But how does that happen? The query string is attached to the
URL, but the filename should be a temp file that we generate.

Looks like a bug when dealing with file: urls, is it our bug or in the xpcom
file routines we use? In any case it's quite different from the reported problem
which involves downloading files from a server, which would use our own filenames.

Comment 10

[dprice (gone)]

Attachment: patch 1

Comment 11

[dprice (gone)]

The patch defaults to the filename to tmp.xpi in the case for non-chrome installs.

Filed bug 137911 to handle the case for chrome installs.  The chrome case is
extremely rare because you must trigger the download through javascript.  It is
also more complicated to fix because we want to have a meaningful filename.

Comment 12

[Syd Logan]

+            tempFilename.Assign(NS_LITERAL_STRING("tmp.xpi")); 

+            temp->AppendUnicode(tempFilename.get());

does tempFilename.get() yield Unicode? Or do we need to extract from
tempFilename and convert to Unicode before doing the append?

Comment 13

[Curt Patrick (gone)]

Comment on attachment 79580 [details] [diff] [review]
patch 1

r=curt

Comment 14

[dprice (gone)]

It is my understanding that AppendUnicode handles all of the required conversions.
Is that right Dan?

Comment 15

[Daniel Veditz [:dveditz] out until Nov 10]

AppendUnicode() handles all the conversions, and tempFilename is an nsString
(not nsCString) so it's also Unicode. However, now that we're no longer dealing
with user-supplied base names we're doing a lot of extra work :-)

I haven't yet looked at the patch (will in a moment) but the snippet syd quoted
you could just do 
  temp->Append("tmp.xpi");
now that we use a fixed filename with all safe chars.

Comment 16

[Daniel Veditz [:dveditz] out until Nov 10]

Comment on attachment 79580 [details] [diff] [review]
patch 1

>+            nsString tempFilename;
>+            tempFilename.Assign(NS_LITERAL_STRING("tmp.xpi")); 
>             nsCOMPtr<nsILocalFile> temp;
>             directoryService->Get(NS_OS_TEMP_DIR,
>                                   NS_GET_IID(nsIFile),
>                                   getter_AddRefs(temp));
>-            temp->AppendUnicode(leaf.get());
>+            temp->AppendUnicode(tempFilename.get());

As long as we're changing this code let's add a null check for temp
before we dereference it, or check the rv of the directoryService
call.

Comment 17

[dprice (gone)]

Attachment: patch 2

Comment 18

[Syd Logan]

Looks good to me, can you describe the testing behind this? 

Comment 19

[Daniel Veditz [:dveditz] out until Nov 10]

Comment on attachment 79843 [details] [diff] [review]
patch 2

Do file:// urls with query strings work? This fix won't touch that, but
nsIFileURL might be doing the right thing already. Jimmy, could you test and
file a bug as appropriate?

For this patch, sr=dveditz

Comment 20

[dprice (gone)]

landed on the trunk

Comment 21

[dprice (gone)]

testing included both test cases listed in the bug, and it was in my tree when I
built and ran mozilla and netscape installers.  But the installers don't use
query strings so they didn't run the code.

Comment 22

[scottputterman]

adding adt1.0.0+.  Please check into the branch as soon as possible and add the
fixed1.0.0 keyword.

Comment 23

[Jimmy Lee]

Build: 2002-04-23-10-trunk(WIN), 2002-04-23-09-trunk(LINUX),
2002-04-23-08-trunk(MAC 10.x, MAC 9.x)

Checked trunk builds for all platforms.  This is working as expected.  Marking
Verified for TRUNK.

Comment 24

[Scott Collins]

Comment on attachment 79843 [details] [diff] [review]
patch 2

technically, this needs an r= before I can approve it for anything, let alone
the branch.  Can you get an r= and resubmit?

Comment 25

[Scott Collins]

Comment on attachment 79843 [details] [diff] [review]
patch 2

(by decision of drivers, we'll assume the r= rolled forward) a=scc for checkin
to the mozilla 1.0 branch

Comment 26

[Jimmy Lee]

Build: 2002-04-25-08-1.0.0(WIN), 2002-04-25-05-1.0.0(MAC),
2002-04-25-08-1.0.0(LINUX)

Looks good on all platforms.  Adding keyword verified1.0.0.