1354590 - Add `temporary` flag to runtime.onInstalled details

Status: RESOLVED FIXED

(WebExtensions :: Developer Tools, enhancement, P5)

Description

[Tomislav Jovanovic :zombie]

Useful for turning on debugging features, like console.logs.

Comment 1

[Tushar Saini (:shatur)]

Hey Tomislav!

I would like to work on this. Please, can you elaborate a little more on what needs to be done?

Thanks!!

Comment 2

[Tomislav Jovanovic :zombie]

Hi Tushar, I've assigned the bug to you.

This would require a few things:
 - modify the AddonInstall.startInstall() method somewhere around [1], 
 - so that extraProperties includes a property named `temporarilyInstalled`, similar to [2],
 - which will be forwarded through `addonData` to the `onInstalled` implementation [3],
 - where you can then pass the flag as a property named `temporary`,
 - and finally, add a test to [4].

Let me know if you have any more questions.


1) http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#5912
2) http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#7342
3) http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-runtime.js#44\
4) http://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled_and_onStartup.js

Comment 3

[Tomislav Jovanovic :zombie]

Actually, the above might not be 100% right, I'll post details after I investigate some more.

Comment 4

[Tomislav Jovanovic :zombie]

(In reply to Tomislav Jovanovic :zombie from comment #2)
>  - modify the AddonInstall.startInstall() method somewhere around [1], 
>
> 1) http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#5912

This should be:
 - modify installAddonFromLocation() method somewhere around [1].

1) http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4111

Comment 5

[Tushar Saini (:shatur)]

Attachment: Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

Review commit: https://reviewboard.mozilla.org/r/135184/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135184/

Comment 6

[Tushar Saini (:shatur)]

Hey!

Sorry for late response, was a little bit busy.

I am not able to understand, what we want to accomplish in this bug. Please can you elobrate?

>  - where you can then pass the flag as a property named `temporary`,

Also, when we talk about "temporary" flag, is it similar to "reason"?

On basis of your instruction, done some changes, but not sure if I am in right direction. Please have a look.

Thanks!!

Comment 7

[Tomislav Jovanovic :zombie]

Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

https://reviewboard.mozilla.org/r/135184/#review138530

Thanks, this is in the right direction, but not quite there.

::: toolkit/components/extensions/ext-runtime.js:44
(Diff revision 1)
>                  if (AddonManagerPrivate.browserUpdated) {
>                    fire.sync({reason: "browser_update"});
>                  }
>                  break;
>                case "ADDON_INSTALL":
> -                fire.sync({reason: "install"});
> +                fire.sync({reason: "install", temporary: extension.addonData.temporarilyInstalled});

The `temporarilyInstalled` flag will only be present when it's true, so you should ensure `temporary` is boolean:
```
    temporary: !!extension.addonData.temporarilyInstalled
```

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4138
(Diff revision 1)
>          }
>          let uninstallReason = installReason;
>  
>          extraParams.newVersion = newVersion;
>          extraParams.oldVersion = oldVersion;
> +        extraParams.temporarilyInstalled = this.temporarilyInstalled;

I don't think this will work.  Did you try it?

You probably need to compare the `aInstallLocation` argument to `TemporaryInstallLocation`.  Also, this likely shouldn't be inside the two if statements, as they are about calling the `shutdown` method.  The `install` is called a dozen lines below.

Comment 8

[Tomislav Jovanovic :zombie]

> I am not able to understand, what we want to accomplish in this bug. Please
> can you elobrate?
We want the extension to be able to know if it was being installed as a temporary extension through about:debugging.


> Also, when we talk about "temporary" flag, is it similar to "reason"?
Yes, somewhat similar.  Extension can use `reason` to see why it was installed/updated, and `temporary` to see if that install was, well, temporary.

Comment 9

[Tushar Saini (:shatur)]

Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135184/diff/1-2/

Comment 10

[Tushar Saini (:shatur)]

Hey!

I have made required changes. Please have a look and let me know if any further modifications is required.

I do not know for sure, but from my past experience, I think we need to update the schema of runtime [1], as I think temporary is a parameter of onInstalled event. Am I wrong?

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/schemas/runtime.json#464

Regards

Comment 11

[Tomislav Jovanovic :zombie]

Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

https://reviewboard.mozilla.org/r/135184/#review139806

This is close, but you are missing a test for when the flag is true.  To do that, you need to pass `useAddonManager: "permanent"` when loading the extension.  And yeah, you should add the flag to the json schema, and mark it optional.

::: toolkit/components/extensions/ext-runtime.js:40
(Diff revision 2)
>          onInstalled: new SingletonEventManager(context, "runtime.onInstalled", fire => {
>            let listener = () => {
>              switch (extension.startupReason) {
>                case "APP_STARTUP":
>                  if (AddonManagerPrivate.browserUpdated) {
> -                  fire.sync({reason: "browser_update"});
> +                  fire.sync({reason: "browser_update", temporary: !!extension.addonData.temporarilyInstalled});

I don't think an addon can be temporarily installed on APP_STARTUP.

::: toolkit/components/extensions/ext-runtime.js:44
(Diff revision 2)
>                  if (AddonManagerPrivate.browserUpdated) {
> -                  fire.sync({reason: "browser_update"});
> +                  fire.sync({reason: "browser_update", temporary: !!extension.addonData.temporarilyInstalled});
>                  }
>                  break;
>                case "ADDON_INSTALL":
> -                fire.sync({reason: "install"});
> +                fire.sync({reason: "install", temporary: !!extension.addonData.temporarilyInstalled});

nit: you can calculate the local value `temporary` once, and use it in both places.

::: toolkit/components/extensions/ext-runtime.js:47
(Diff revision 2)
>                  break;
>                case "ADDON_INSTALL":
> -                fire.sync({reason: "install"});
> +                fire.sync({reason: "install", temporary: !!extension.addonData.temporarilyInstalled});
>                  break;
>                case "ADDON_UPGRADE":
> -                fire.sync({reason: "update", previousVersion: extension.addonData.oldVersion});
> +                fire.sync({reason: "update", temporary: !!extension.addonData.temporarilyInstalled,

Why are you removing previousVersion here?  Don't do that.

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled_and_onStartup.js:65
(Diff revision 2)
> -function* expectEvents(extension, {onStartupFired, onInstalledFired, onInstalledReason, onInstalledPrevious}) {
> +function* expectEvents(extension, {onStartupFired, onInstalledFired, onInstalledReason, onInstalledTemporary, onInstalledPrevious}) {
>    extension.sendMessage("get-on-installed-details");
>    let details = yield extension.awaitMessage("on-installed-details");
>    if (onInstalledFired) {
>      equal(details.reason, onInstalledReason, "runtime.onInstalled fired with the correct reason");
> +    equal(details.temporary, onInstalledTemporary, "runtime.onInstalled fired with the correct temporary flag");

details.temporary is optional, so we should make onInstalledTemporary optional as well, and default it to false.  In other words, just convert both to boolean before comparing them.

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled_and_onStartup.js:142
(Diff revision 2)
>  
>    yield expectEvents(extension, {
>      onStartupFired: false,
>      onInstalledFired: true,
>      onInstalledReason: "install",
> +    onInstalledTemporary: false,

And given that we made it optional above, we don't actually need to add it when it is false, here or below.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4190
(Diff revision 2)
>      }
>  
>      let file = addon._sourceBundle;
>  
>      XPIProvider._addURIMapping(addon.id, file);
> +    extraParams.temporarilyInstalled = (aInstallLocation == TemporaryInstallLocation);

nits: please use ===, no need for parentheses, and I still think this would be better around line #4145, so that it can be picked up in the shutdown/uninstall case too.

Comment 12

[Tomislav Jovanovic :zombie]

> you need to pass `useAddonManager: "permanent"` 

oops, I meant "temporary".

Comment 13

[Tushar Saini (:shatur)]

Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135184/diff/2-3/

Comment 14

[Tomislav Jovanovic :zombie]

Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

https://reviewboard.mozilla.org/r/135184/#review139862

Nice work Tushar!  

I'm asking Andrew to double-check in case I missed something, but r=me.

::: toolkit/components/extensions/ext-runtime.js:46
(Diff revision 3)
>                  if (AddonManagerPrivate.browserUpdated) {
>                    fire.sync({reason: "browser_update"});
>                  }
>                  break;
>                case "ADDON_INSTALL":
> -                fire.sync({reason: "install"});
> +                fire.sync({reason: "install", temporary: temporary});

nit: you can just use property value shorthand [1], here and below, like so:

    fire.sync({reason: "install", temporary});

1) https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Operators/Object_initializer#New_notations_in_ECMAScript_2015

Comment 15

[Tushar Saini (:shatur)]

Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135184/diff/3-4/

Comment 16

[Andrew Swan [:aswan]]

Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

https://reviewboard.mozilla.org/r/135184/#review140344

Looking good but there should also be a test case that for regular installation, the temporary flag is false (this can probably just be added to an existing test)

::: toolkit/components/extensions/ext-runtime.js:49
(Diff revision 4)
>                  break;
>                case "ADDON_INSTALL":
> -                fire.sync({reason: "install"});
> +                fire.sync({reason: "install", temporary});
>                  break;
>                case "ADDON_UPGRADE":
> -                fire.sync({reason: "update", previousVersion: extension.addonData.oldVersion});
> +                fire.sync({reason: "update", temporary, previousVersion: extension.addonData.oldVersion});

this is getting big enough, please split this up with each property on its own line

::: toolkit/components/extensions/schemas/runtime.json:480
(Diff revision 4)
>                  "optional": true,
>                  "description": "Indicates the previous version of the extension, which has just been updated. This is present only if 'reason' is 'update'."
>                },
> +              "temporary": {
> +                "type": "boolean",
> +                "optional": true,

why is this marked optional?  you're always sending it...

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled_and_onStartup.js:65
(Diff revision 4)
> -function* expectEvents(extension, {onStartupFired, onInstalledFired, onInstalledReason, onInstalledPrevious}) {
> +function* expectEvents(extension, {onStartupFired, onInstalledFired, onInstalledReason, onInstalledTemporary, onInstalledPrevious}) {
>    extension.sendMessage("get-on-installed-details");
>    let details = yield extension.awaitMessage("on-installed-details");
>    if (onInstalledFired) {
>      equal(details.reason, onInstalledReason, "runtime.onInstalled fired with the correct reason");
> +    equal(!!details.temporary, !!onInstalledTemporary, "runtime.onInstalled fired with the correct temporary flag");

just give onInstalledTemporary a default value instead of using `!!` please

Comment 17

[Tomislav Jovanovic :zombie]

Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

https://reviewboard.mozilla.org/r/135184/#review140344

The updated `expectEvents` always checks the value of `details.temporary`, and expects false by default, so I thought this was covered by existing tests.  Or did you have something else in mind?

Comment 18

[Tomislav Jovanovic :zombie]

Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

https://reviewboard.mozilla.org/r/135184/#review140350

::: toolkit/components/extensions/schemas/runtime.json:480
(Diff revision 4)
>                  "optional": true,
>                  "description": "Indicates the previous version of the extension, which has just been updated. This is present only if 'reason' is 'update'."
>                },
> +              "temporary": {
> +                "type": "boolean",
> +                "optional": true,

It's not sent on APP_STARTUP.  I guess we could just send `false`, but I didn't like it since I don't think it could ever be true in that case.

Comment 19

[Andrew Swan [:aswan]]

Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

https://reviewboard.mozilla.org/r/135184/#review140344

Duh, of course.  That's kind of non-obvious though, how about forgetting the line-item comment about giving it a default value and just passing it in explicitly, its like a dozen call sites.

Comment 20

[Tushar Saini (:shatur)]

Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135184/diff/4-5/

Comment 21

[Andrew Swan [:aswan]]

Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

https://reviewboard.mozilla.org/r/135184/#review141246

Nice, thanks!

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled_and_onStartup.js:65
(Diff revision 5)
> -function* expectEvents(extension, {onStartupFired, onInstalledFired, onInstalledReason, onInstalledPrevious}) {
> +function* expectEvents(extension, {onStartupFired, onInstalledFired, onInstalledReason, onInstalledTemporary, onInstalledPrevious}) {
>    extension.sendMessage("get-on-installed-details");
>    let details = yield extension.awaitMessage("on-installed-details");
>    if (onInstalledFired) {
>      equal(details.reason, onInstalledReason, "runtime.onInstalled fired with the correct reason");
> +    equal(!!details.temporary, !!onInstalledTemporary, "runtime.onInstalled fired with the correct temporary flag");

nit: the !! isn't necessary on onInstalledTemporary

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled_and_onStartup.js:328
(Diff revision 5)
>  
>    yield extension.markUnloaded();
>    yield promiseShutdownManager();
>  });
> +
> +add_task(function* test_when_addon_installed_temporarily() {

nit: this name could be shortened...

Comment 22

[Tushar Saini (:shatur)]

Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135184/diff/5-6/

Comment 23

[Tushar Saini (:shatur)]

Hey Tomislav!

I have made requested changes. I think this will be landable. Please have a look & do tell me if further modification is needed. Thanks for mentoring!!

Comment 24

[Tomislav Jovanovic :zombie]

Good job Tushar.  

The only thing left is to ask for landing by adding the checkin-needed keyword.

Comment 25

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 666f6453f70f -d c9732a51552b: rebasing 395638:666f6453f70f "Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details. r=aswan,zombie" (tip)
merging toolkit/components/extensions/ext-runtime.js
merging toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled_and_onStartup.js
merging toolkit/mozapps/extensions/internal/XPIProvider.jsm
warning: conflicts while merging toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled_and_onStartup.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 26

[Tushar Saini (:shatur)]

Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135184/diff/6-7/

Comment 27

[Tushar Saini (:shatur)]

Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135184/diff/7-8/

Comment 28

[Tushar Saini (:shatur)]

Hey Tomislav!

Test have been modified to use async/await from Task.jsm/yield, so I've modified the test. Can you please have a look before flagging checkin flag??

Thanks!!

Comment 29

[Tomislav Jovanovic :zombie]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84b968cfde2e&selectedJob=98998986
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2719ee127b46fba206b9789d518a1be525dbc9e

Comment 30

[Tomislav Jovanovic :zombie]

Looks good.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/830cc1aae18f
Add 'temporary' flag to runtime.onInstalled details. r=aswan,zombie

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/830cc1aae18f

Comment 33

[Caitlin Neiman [:caitmuenster]]

Thank you, Tushar! Your contribution has been added to our recognition wiki at https://wiki.mozilla.org/Add-ons/Contribute/Recognition#May_2017. Your Mozillians profile has also been vouched. :) 

Welcome onboard! I look forward to seeing you around!

Comment 34

[Tushar Saini (:shatur)]

Thanks for the vouching!!

Comment 35

[Will Bamberg [:wbamberg]]

-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/onInstalled

Marking dev-doc-complete, but let me know if we need anything else.