540558 - window.InstallTrigger.install exists but won't work (and won't throw error) since 3.6

Status: RESOLVED FIXED

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

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: testcase with both way to access the object

Reproducibly: Always.

Steps:
*) invoke add-on installation by install(), accessed using "window.InstallTrigger.install()" rather than just "InstallTrigger.install()".

Result:
* both ways works perfectly in 3.5, but window.InstallTrigger one failed under 3.6 without throwing any errors. Also, the script follows stop executing.

Note:
Both InstallTrigger object exists (try alert()) in 3.6 and 3.5. 
Quick test by turning off jit (chrome and content) w/o restarting won't fix (should I restart after turning off jit in chrome?)

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Turns out I was wrong in bug 475934, we still need to create the native object in this cases. Though I'm not totally sure why it's different for plain InstallTrigger vs. window.InstallTrigger this partial backout does fix it. Added an automated test for good measure.

Comment 2

[Daniel Veditz [:dveditz]]

Comment on attachment 422487 [details] [diff] [review]
patch rev 1

>+++ b/xpinstall/tests/bug540558.html
>+function installCallback(url, status) {
>+  document.getElementById("status").textContent = status;
>+}
>+
>+function startInstall() {
>+  window.InstallTrigger.install({
>+    "Unsigned XPI": "http://example.com/browser/xpinstall/tests/unsigned.xpi"
>+  });
>+}

You define installCallback, but don't use it in the call to install(). Is that some magic in the test harness?

other than that r=dveditz on the installTrigger patch.

Comment 3

[Dave Townsend [:mossop]]

(In reply to comment #2)
> (From update of attachment 422487 [details] [diff] [review])
> >+++ b/xpinstall/tests/bug540558.html
> >+function installCallback(url, status) {
> >+  document.getElementById("status").textContent = status;
> >+}
> >+
> >+function startInstall() {
> >+  window.InstallTrigger.install({
> >+    "Unsigned XPI": "http://example.com/browser/xpinstall/tests/unsigned.xpi"
> >+  });
> >+}
> 
> You define installCallback, but don't use it in the call to install(). Is that
> some magic in the test harness?
> 
> other than that r=dveditz on the installTrigger patch.

No just some cruft I failed to remove when I copied the testcase from a different test. I'll remove it before landing.

Comment 4

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/3d5cb3818b55

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 422487 [details] [diff] [review]
patch rev 1

Would be nice to fix this regression on the branch, should be no risk.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 422487 [details] [diff] [review]
patch rev 1

a=beltzner for 1.9.2.4

Comment 7

[Dave Townsend [:mossop]]

Pushed to branch: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/500f01eb0770

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.2 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.4pre) Gecko/20100412 Namoroka/3.6.4pre (.NET CLR 3.5.30729) using attached testcase. window.InstallTrigger.install works now.