InstallTrigger.getVersion gone in Firefox 1.0.3

VERIFIED FIXED

Status

defect
VERIFIED FIXED
14 years ago
3 years ago

People

(Reporter: pete, Assigned: pete)

Tracking

(4 keywords)

Bug Flags:
blocking1.7.12 +
blocking-aviary1.0.5 -
blocking-aviary1.0.7 +
blocking1.8b3 -
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

14 years ago
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

Updated

14 years ago
Assignee: bugs → xpi-engine
Component: Installer → Installer: XPInstall Engine
Product: Firefox → Core
QA Contact: bugzilla
Version: unspecified → 1.7 Branch
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.)
Assignee: xpi-engine → dveditz
Flags: blocking1.7.8+
Flags: blocking-aviary1.0.4+
Keywords: regression
Hm, I was testing on Windows. Shouldn't matter for this object, though.
Flags: blocking1.7.8?
Flags: blocking1.7.8+
Flags: blocking-aviary1.0.4?
Flags: blocking-aviary1.0.4+
(Assignee)

Comment 3

14 years ago
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.


(Assignee)

Updated

14 years ago
OS: Linux → Windows XP
Not blocking a security release but does need fixing
Flags: blocking1.8b3?
Flags: blocking1.7.8?
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.0.5?
Flags: blocking-aviary1.0.5-
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?
*** Bug 298558 has been marked as a duplicate of this bug. ***

Updated

14 years ago
Flags: blocking1.8b4+
Flags: blocking1.8b3?
Flags: blocking1.8b3-
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.1+

Comment 7

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

/cb
Flags: blocking1.8b4-
Flags: blocking1.8b4+
Flags: blocking-aviary1.5-
Flags: blocking-aviary1.5+

Comment 8

14 years ago
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.
Flags: blocking1.8b4?
Flags: blocking1.8b4-
Flags: blocking-aviary1.0.7?
(Assignee)

Comment 9

14 years ago
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

14 years ago
(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

14 years ago
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.
(Assignee)

Comment 12

14 years ago
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.

Assignee: dveditz → pete
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?
(Assignee)

Comment 14

14 years ago
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.
(Assignee)

Comment 15

14 years ago
As I suspected, it is failing here:

 
http://lxr.mozilla.org/seamonkey/source/xpinstall/src/nsJSInstallTriggerGlobal.cpp#703
Status: NEW → ASSIGNED
(Assignee)

Comment 16

14 years ago
Pass One:

  Assign a valid return value before returning from func ...
(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 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.
Attachment #190793 - Flags: review-
(Assignee)

Comment 19

14 years ago
Pulling out returns

-  if (!nativeThis)
-    return JS_FALSE;
Attachment #190793 - Attachment is obsolete: true
(Assignee)

Comment 20

14 years ago
Posted file test.xpi
Test XPI
(Assignee)

Comment 21

14 years ago
W/ latest patch, getVersion call now returns null and script execution is not
stopped which was the real show stopper here. 
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 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)
Attachment #190846 - Flags: review+
(Assignee)

Comment 24

14 years ago
Same patch using:

  cvs diff -uws -p6
Attachment #190846 - Attachment is obsolete: true
Comment on attachment 190900 [details] [diff] [review]
xpinstall/src/nsJSInstallTriggerGlobal.cpp

r=dveditz if you remove the debug printf
Attachment #190900 - Flags: review+
Flags: blocking1.7.12?
Flags: blocking-aviary1.5?
Flags: blocking-aviary1.5-

Updated

14 years ago
Attachment #190900 - Flags: superreview?(dbaron)

Updated

14 years ago
Flags: blocking1.8b4? → blocking1.8b4+
Flags: blocking-aviary1.5?
(Assignee)

Comment 26

14 years ago
Same patch with the printf removed ...

I believe we just need someone to sr this patch
Attachment #190900 - Attachment is obsolete: true
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.
Attachment #192257 - Flags: superreview+
Attachment #192257 - Flags: review?(timeless)

Comment 28

14 years ago
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

Updated

14 years ago
Attachment #192257 - Flags: review?(timeless) → review+
(Assignee)

Comment 29

14 years ago
Fix checked into the trunk, thanks for the review gents.

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

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
(Assignee)

Comment 31

14 years ago
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

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

/be
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 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
Attachment #192257 - Flags: approval1.8b4+
(Assignee)

Comment 35

14 years ago
(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

14 years ago
Is this bug fixed? Can we mark it so?
(Assignee)

Comment 37

14 years ago
(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
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 38

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

/cb
(Assignee)

Comment 39

14 years ago
(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 ...
Attachment #192257 - Flags: approval1.7.12?
Attachment #192257 - Flags: approval-aviary1.0.7?
Flags: blocking1.7.12?
Flags: blocking-aviary1.0.7?

Updated

14 years ago
Flags: blocking-aviary1.0.7? → blocking-aviary1.0.7+
Flags: blocking1.7.12? → blocking1.7.12+
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).
Attachment #192257 - Flags: approval1.7.12?
Attachment #192257 - Flags: approval1.7.12+
Attachment #192257 - Flags: approval-aviary1.0.7?
Attachment #192257 - Flags: approval-aviary1.0.7+
Flags: blocking1.7.13?
Flags: blocking-aviary1.0.8?

Comment 42

14 years ago
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

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

Verified.
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.