291178 - InstallTrigger.getVersion gone in Firefox 1.0.3

Status: VERIFIED FIXED

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

Description

[Pete Collins (MDG)]

Using Firefox 1.0.3

InstallTrigger.getVersion results in a script error.

typeof(InstallTrigger) == object

typeof(InstallTrigger.getVersion) == undefined

Anyone know of any regression in Firefox 1.0.3 that has broken this?

It works fine in Firefox 1.0.1 and 1.0.2

Comment 1

[Daniel Veditz [:dveditz]]

bug 290162 affected the InstallTrigger object. That said, I can't reproduce
this, at least not from a browser context. Are you trying to access
InstallTrigger from an install script? (That should work too, but I haven't
tried it.)

Comment 2

[Daniel Veditz [:dveditz]]

Hm, I was testing on Windows. Shouldn't matter for this object, though.

Comment 3

[Pete Collins (MDG)]

This is from install.js context.

Steps to reproduce.

1. launch FF 1.0.3
2. install an xpi package that calls InstallTrigger.getVersion func
3. installation will return w/ "script error"

Do note I am using signed xpi packages. I haven't tested w/ an unsigned package.

Also as a test I added the code below and typeof(InstallTrigger.getVersion)
resulted in undefined. I tried to iterate the InstallTrigger object for
properties and came up w/ nothing, no properties.

Let me know if you need me to provide a test package as I had to remove the call
because this bug was blocking our production.

Also I do want to note that XPInstall is critical to our work. The extension
manager doesn't provide the flexibility we need for installing moz apps,
packages, and extensions. For example we are running client production
installers from XPInstall, some packages require global chrome, creating new
folders in various locations, multi package/dependency installs, etc. All of
this is impossible w/ EM. Also XPInstall works for the suite and FF. Em is FF
only ...

So I have an interest in making sure XPInstall sticks around and would love to
see it evolve as well.

For example, improvements to the UI.

1. Stronger differentiation in the display between signed and unsigned packages.
2. The ability to add some branding to the install dialog
3. A smoother cleaner install process
4. Installing w/out having to restart

We also implement uninstall functionality in jsLib and it works wonderful, you
don't need to restart for the uninstall.

Comment 4

[Daniel Veditz [:dveditz]]

Not blocking a security release but does need fixing

Comment 5

[alexander :surkov (:asurkov)]

In mozilla 1.8b2 (trunk 2005-05-30-07) typeof(InstallTrigger.getVersion)==func
but when I try to use InstallTrigger.getVersion() from install script then I get
exception "script error". Is the same bug or something else?

Comment 6

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

*** Bug 298558 has been marked as a duplicate of this bug. ***

Comment 7

[Chris Beard]

minor feature of dying mechanism, increasingly irrelevant.  nice to fix, but not
needed with extension manager.

/cb

Comment 8

[Benjamin Smedberg]

cbeard, InstallTrigger is a very important backwards-compatibility mechanism and
I cannot believe we're letting this slide either on our stable branch or the
release.

Comment 9

[Pete Collins (MDG)]

Guys, I am stunned at the attitude towards xpinstall in general. 

Mozilla needs both the Extension Manager and XPInstall and not just one or the
other.

I presume the poor attitude towards the install.js/xpinstall method is just
based on not knowing both systems intimately and the importance they both
provide to Mozilla application developers and extension authors.

If you ever wrote a Mozilla Application or many different types of extensions,
you would quickly see the limitations of the Extension Manager.

InstallTrigger is not only needed for backwards compatibility but it is needed
for forwards compatibility as well. 

Also getting rid of xpinstall means a complete rewrite of how a Mozilla based
distribution is installed onto a system. 

I have grown quite fond of the Extension Manager for installing simple
extensions and add-ons but it would be foolish to think that in its current
state it could replace XPInstall.

Can someone please fill me in on the future of XPInstall? 

Is there work being done on the EM that will make it more robust?

Thanks 

Comment 10

[Chris Beard]

(In reply to comment #8)
> cbeard, InstallTrigger is a very important backwards-compatibility mechanism and
> I cannot believe we're letting this slide either on our stable branch or the
> release.

we'd certainly be open to someone stepping up to provide a patch in the 1.8b4
timeframe. (but who?)  i was only relaying dveditz's comments as we
reviewed/triaged this bug, which had no owner and no forward progress, and in
considering it's relative priority.

/cb

Comment 11

[Benjamin Smedberg]

InstallTrigger.getVersion has been a part of the documented XPInstall API since
Netscape 6 or so, and I don't want us to go down the road of introducing
significant API regressions and then bumping them just because we don't have
somebody to fix them.

Comment 12

[Pete Collins (MDG)]

Ok, I didn't realize that it was a resource issue. I thought there might be a
pre-established decision to start to phase out xpinstall which really scares the
heck out of me, not because I fear change (which I do) but because EM is too
limited in it's present state to deep six xpinstall. 

Also a change that big really should be discussed.

Comment 13

[Doug Turner (:dougt)]

how was this regression cause?  does anyone have a bonsai query for changes
between 1.0.3 and 1.0.2 (which I presume worked).  Does this have anything to do
with the whitelist changes?

Comment 14

[Pete Collins (MDG)]

Doug, I presume it was this patch that caused the regression ...

https://bugzilla.mozilla.org/attachment.cgi?id=180631

Looks like a lot of added checks for js contexts in there. I will have a look
tomorrow.

Comment 15

[Pete Collins (MDG)]

As I suspected, it is failing here:

 
http://lxr.mozilla.org/seamonkey/source/xpinstall/src/nsJSInstallTriggerGlobal.cpp#703

Comment 16

[Pete Collins (MDG)]

Attachment: xpinstall/src/nsJSInstallTriggerGlobal.cpp

Pass One:

  Assign a valid return value before returning from func ...

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #16)
> Created an attachment (id=190793) [edit]
> xpinstall/src/nsJSInstallTriggerGlobal.cpp
> 
> Pass One:
> 
>   Assign a valid return value before returning from func ...

Pete, the *rval out parameter is not used if the native function returns
JS_FALSE, so this change is not necessary or desirable.

Cc'ing dveditz, who can explain where getVersion moved, and why; I forget the
details and I'm on paternity leave.

/be

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 190793 [details] [diff] [review]
xpinstall/src/nsJSInstallTriggerGlobal.cpp

r-

Looks like the real problem is that when Brendan changed JS_GetInstance to
JS_GetInstancePrivate he also added a return after failure. The old code, which
is still there, expected a first-time failure for InstallTrigger and has a
CreateNativeObject() call. This is now bypassed.

I think the fix is just to remove the "if (!nativeThis) return JS_FALSE" lines
and let them fall through to the CreateNativeObject() bits (which is from bug
6453 to add InstallTrigger to the xpinstall context).

Thanks for tracking down the regressing check-in.

Comment 19

[Pete Collins (MDG)]

Attachment: xpinstall/src/nsJSInstallTriggerGlobal.cpp

Pulling out returns

-  if (!nativeThis)
-    return JS_FALSE;

Comment 20

[Pete Collins (MDG)]

Attachment: test.xpi

Test XPI

Comment 21

[Pete Collins (MDG)]

W/ latest patch, getVersion call now returns null and script execution is not
stopped which was the real show stopper here. 

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 190846 [details] [diff] [review]
xpinstall/src/nsJSInstallTriggerGlobal.cpp

This patch is not quite the cat's meow yet -- it hides reported errors (thrown
as exceptions, possibly just wasteful) due to the non-null argv parameter to
JS_GetInstancePrivate.

I don't believe the right answer is to pass null in all those cases, though I
may be misremembering.

Sorry for the regression, I hope I can still add a useful comment here while
baby-juggling.

/be

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 190846 [details] [diff] [review]
xpinstall/src/nsJSInstallTriggerGlobal.cpp

r=dveditz

Please use more context and the -p option (e.g. cvs diff -p6)

Comment 24

[Pete Collins (MDG)]

Attachment: xpinstall/src/nsJSInstallTriggerGlobal.cpp

Same patch using:

  cvs diff -uws -p6

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 190900 [details] [diff] [review]
xpinstall/src/nsJSInstallTriggerGlobal.cpp

r=dveditz if you remove the debug printf

Comment 26

[Pete Collins (MDG)]

Attachment: xpinstall/src/nsJSInstallTriggerGlobal.cpp

Same patch with the printf removed ...

I believe we just need someone to sr this patch

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 192257 [details] [diff] [review]
xpinstall/src/nsJSInstallTriggerGlobal.cpp

how about sr=me and get r= from someone else, might be easier. Timeless has
been mucking about in here with this low-level js stuff. Or dougt.

Comment 28

[timeless]

Comment on attachment 192257 [details] [diff] [review]
xpinstall/src/nsJSInstallTriggerGlobal.cpp

i knew this patch looked familiar: bug 295854 comment 6

passing 0 instead of argv seems correct based on my reading of the file and the
api

Comment 29

[Pete Collins (MDG)]

Fix checked into the trunk, thanks for the review gents.

Please instruct on what branches should be hit as well ...

Comment 30

[Brendan Eich [:brendan]]

Comment on attachment 192257 [details] [diff] [review]
xpinstall/src/nsJSInstallTriggerGlobal.cpp

> PR_STATIC_CALLBACK(JSBool)
> InstallTriggerGlobalUpdateEnabled(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
> {
>   nsIDOMInstallTriggerGlobal *nativeThis = (nsIDOMInstallTriggerGlobal*)
>-    JS_GetInstancePrivate(cx, obj, &InstallTriggerGlobalClass, argv);
>-  if (!nativeThis)
>-    return JS_FALSE;
>+    JS_GetInstancePrivate(cx, obj, &InstallTriggerGlobalClass, 0);
> 
>   *rval = JSVAL_FALSE;
> 
>   if (nsnull == nativeThis  &&  (JS_FALSE == CreateNativeObject(cx, obj, &nativeThis)) )
>     return JS_TRUE;

This pattern repeats over and over, and it's pretty bogus if CreateNativeObject
returning false means it threw an error-as-exception (or reported OOM).  The
return JS_TRUE means that error is ignored.

If anyone wants to clean this up, feel free to get rid of the silly (JS_FALSE
== ...) training wheels.

Who approved this patch landing, anyway?

/be

Comment 31

[Pete Collins (MDG)]

Existing code was horribly broken causing all xpi packages that call on any of
these InstallTrigger methods to just fail because js script execution is halted
with a "script error". BAD

The patch checked in prevents this.  BETTER

Please feel free to improve upon the patch checked in but just don't revert it
back to brokenness. 

Thanks

Comment 32

[Brendan Eich [:brendan]]

No one's reverting nothin'.  Not to worry.

/be

Comment 33

[Brendan Eich [:brendan]]

Pete, you may see some xpinstall luvin' from yours truly over in bug 301265 -- I
won't break it, especially if you help test it!  Thanks again for your help here.

/be

Comment 34

[Brendan Eich [:brendan]]

Comment on attachment 192257 [details] [diff] [review]
xpinstall/src/nsJSInstallTriggerGlobal.cpp

This was landed, the bug was approved, just noting that the patch has
(retrospective) approval.

/be

Comment 35

[Pete Collins (MDG)]

(In reply to comment #33)
> Pete, you may see some xpinstall luvin' from yours truly over in bug 301265 -- I
> won't break it, especially if you help test it!  Thanks again for your help here.

Great, I will certainly help test being that I rely heavily on XPInstall.

Comment 36

[Benjamin Smedberg]

Is this bug fixed? Can we mark it so?

Comment 37

[Pete Collins (MDG)]

(In reply to comment #36)
> Is this bug fixed? Can we mark it so?

Checked in on the trunk, not any branches.

The resulting script error is now fixed. We still don't get a version but I
believe that would be another bug.

Marking FIXED

Comment 38

[Chris Beard]

Did it land on the trunk before or after the branch for 1.8?

/cb

Comment 39

[Pete Collins (MDG)]

(In reply to comment #38)
> Did it land on the trunk before or after the branch for 1.8?

It landed at 2005-08-11 15:43 PDT (last Thursday)

I don't know when branch for 1.8 was cut ...

Comment 40

[timeless]

http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/xpinstall/src/nsJSInstallTriggerGlobal.cpp&rev=MOZILLA_1_8_BRANCH

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to AVIARY_1_0_1_2005124_BRANCH and MOZILLA_1_7_BRANCH (by using cvs
up -j to get exactly what was checked in to the trunk).

Comment 42

[Bob Clary [:bc] (inactive)]

verifed no error installing into Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.7.12) Gecko/20050914 Firefox/1.0.7

Comment 43

[Mike Schroepfer]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050913
Firefox/1.0.7

Verified.