263366 - execute() doesn't check if exection succeeded or not, and claims it did even if that's not the case

Status: RESOLVED FIXED

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

Description

[Johnny Stenback (:jst)]

If an xpi file calls execute("foo") and we fail to execute "foo", the xpinstall
process happily pretends everything worked fine even if it failed to execute
"foo", or if execution of "foo" failed. This seems rather wrong... Patch coming up.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Catch errors from executing files

Comment 2

[Johnny Stenback (:jst)]

Attachment: testcase that fails to execute().

Comment 3

[Daniel Veditz [:dveditz]]

Comment on attachment 161395 [details] [diff] [review]
Catch errors from executing files

A non-blocking process--a very common case if you're launching a "wrapped
install"--will return the initial mExitValue of -1

We should probably invent a new error code for this, too. Failures of the
"Complete()" methods are all lumped into the return from performInstall().
Script authors would be helped to know that it was an executable that failed.

Comment 4

[Daniel Veditz [:dveditz]]

We probably want to return an error in the File.execute() case as well... patch
coming up to address my issues

Comment 5

[Daniel Veditz [:dveditz]]

Attachment: patch for aviary branch

This patch expands the previous patch to address my comments. Sorry it's an
aviary patch -- my only clean tree at the moment -- but there shouldn't be any
trunk conflicts

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 161418 [details] [diff] [review]
patch for aviary branch

- In nsInstallFileOpItem::NativeFileOpFileExe...

       rv = process->Run(mBlocking, (const char **)&cParams, argcount, nsnull);
+      if (NS_SUCCEEDED(rv) && mBlocking)
+      {
[...]
+      }
+      else
+	 result = nsInstall::EXECUTION_ERROR;

Won't this result in an error for every non-blocking execute() call that goes
through this code?

sr=jst with that taken care of.

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 161418 [details] [diff] [review]
patch for aviary branch

>Index: xpinstall/src/nsInstall.h
>===================================================================

>-            TOO_MANY_CERTIFICATES       = -203,
>+            EXECUTION_ERROR             = -203,

Do we care about the other places where this is defined? (see
http://lxr.mozilla.org/seamonkey/search?string=TOO_MANY_CERTIFICATES)

What jst said.

Comment 8

[Daniel Veditz [:dveditz]]

Attachment: fix review comments

Fixed installer error strings and the File.execute() non-blocking case

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 162166 [details] [diff] [review]
fix review comments

Carrying over r/sr, seeking branch approval. jst wants this for the plugin
finder

Comment 10

[Daniel Veditz [:dveditz]]

Nominating for 1.0 because I think jst needs this: clear nomination if you don't.

Comment 11

[Asa Dotzler [:asa]]

Comment on attachment 162166 [details] [diff] [review]
fix review comments

a=asa for aviary checkin and 1.7 checkin.

Comment 12

[Daniel Veditz [:dveditz]]

Fixed on trunk, 1.7 and aviary branches.

Comment 13

[Johnny Stenback (:jst)]

For the record, this caused a regression on the mac due to a bug in the fix for
bug 263429 where mExitValue was never set to the actual exit value, it was left
at its initial value of -1. Fixed on branches, no bug.

Comment 14

[Johnny Stenback (:jst)]

Oh, and that was fixed on the trunk too.