1284564 - "Update Behavior" in system add-on spec does not describe behavior

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Robert Helmer [:rhelmer]]

The "Update Behavior" section describes ``<updates></updates>`` as removing everything and ``<addons></addons>`` as doing nothing, the opposite is true.

This is correct elsewhere in the document, so it's contradictory currently.

Comment 1

[Robert Helmer [:rhelmer]]

Attachment: Bug 1284564 - correct update behavior section of system add-on spec

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

Comment 2

[Robert Helmer [:rhelmer]]

BTW I just tested and see something odd about the ``<addons></addons>`` case:

* if we have not applied any updates, nothing happens and this is logged:
https://dxr.mozilla.org/mozilla-central/rev/c9a70b64f2faa264296f0cc90d68a2ee2bac6ac5/toolkit/mozapps/extensions/internal/XPIProvider.jsm#3045

* if there *have* been any updates applied, then instead all updates are removed and all default add-ons are disabled, as documented.

Based on the log messages, it's making it all the way to the end of `updateSystemAddons()` in this case.

Fixing this should probably be a separate bug, but should we document the current behavior for now or leave it alone?

Comment 3

[Osmose [:osmose, :mkelly]]

Comment on attachment 8768077 [details]
Bug 1284564 - correct update behavior section of system add-on spec

https://reviewboard.mozilla.org/r/62412/#review59632

::: toolkit/mozapps/extensions/docs/SystemAddons.rst:173
(Diff revision 1)
>  A response from AUS with an empty add-on set will *disable all system add-ons*:
>  
>  .. code-block:: xml
>  
>    <updates>
>      <addons></addons>

Doesn't this example need to be `<updates></updates>` now? Since a missing addons tag disables all the add-ons?

::: toolkit/mozapps/extensions/docs/SystemAddons.rst:197
(Diff revision 1)
>  
>  The other 90% of the time, AUS sends out an empty response:
>  
>  .. code-block:: xml
>  
>    <updates></updates>

Same here, I think with your changes this needs the `<addons></addons>` tag.

Comment 4

[Osmose [:osmose, :mkelly]]

(In reply to Robert Helmer [:rhelmer] from comment #2)
> BTW I just tested and see something odd about the ``<addons></addons>`` case:
> 
> * if we have not applied any updates, nothing happens and this is logged:
> https://dxr.mozilla.org/mozilla-central/rev/
> c9a70b64f2faa264296f0cc90d68a2ee2bac6ac5/toolkit/mozapps/extensions/internal/
> XPIProvider.jsm#3045

Ah, that makes sense, the two empty sets match. Reading through the code again, `addonList` is `null` if the `<addons>` tag is missing, and `[]` if it is present but empty.
 
> * if there *have* been any updates applied, then instead all updates are
> removed and all default add-ons are disabled, as documented.
> 
> Based on the log messages, it's making it all the way to the end of
> `updateSystemAddons()` in this case.

I guess I don't quite understand what `systemAddonLocation.cleanDirectories()` is actually doing. Is it literally deleting all the system add-ons from the profile directory? I expected it to just get rid of temporary directories or something.
 
> Fixing this should probably be a separate bug, but should we document the
> current behavior for now or leave it alone?

Documenting the current behavior I think is the thing to do.

Comment 5

[Robert Helmer [:rhelmer]]

(In reply to Michael Kelly [:mkelly,:Osmose] from comment #4)
> (In reply to Robert Helmer [:rhelmer] from comment #2)
> > BTW I just tested and see something odd about the ``<addons></addons>`` case:
> > 
> > * if we have not applied any updates, nothing happens and this is logged:
> > https://dxr.mozilla.org/mozilla-central/rev/
> > c9a70b64f2faa264296f0cc90d68a2ee2bac6ac5/toolkit/mozapps/extensions/internal/
> > XPIProvider.jsm#3045
> 
> Ah, that makes sense, the two empty sets match. Reading through the code
> again, `addonList` is `null` if the `<addons>` tag is missing, and `[]` if
> it is present but empty.
>  
> > * if there *have* been any updates applied, then instead all updates are
> > removed and all default add-ons are disabled, as documented.
> > 
> > Based on the log messages, it's making it all the way to the end of
> > `updateSystemAddons()` in this case.
> 
> I guess I don't quite understand what
> `systemAddonLocation.cleanDirectories()` is actually doing. Is it literally
> deleting all the system add-ons from the profile directory? I expected it to
> just get rid of temporary directories or something.


It removes outdated sets of update add-ons - each set of add-ons are installed into a subdirectory generated with a UUID: http://searchfox.org/mozilla-central/rev/92616e983b8ad6e99ec148f341e146c3c6fa312a/toolkit/mozapps/extensions/internal/XPIProvider.jsm#8395

`cleanDirectories()` removes files not currently in use or pending use after restart.

Temporary files go in the OS-specified temp locations so they aren't really an issue.


> > Fixing this should probably be a separate bug, but should we document the
> > current behavior for now or leave it alone?
> 
> Documenting the current behavior I think is the thing to do.


OK will do, I think it's definitely confusing as-is and something we should fix but I'll file a separate bug for that.

Comment 6

[Osmose [:osmose, :mkelly]]

Comment on attachment 8768077 [details]
Bug 1284564 - correct update behavior section of system add-on spec

https://reviewboard.mozilla.org/r/62412/#review61132

LGTM, r+

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b1f7a028b0e
correct update behavior section of system add-on spec r=mkelly

Comment 8

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #5)
> (In reply to Michael Kelly [:mkelly,:Osmose] from comment #4)
> > (In reply to Robert Helmer [:rhelmer] from comment #2)
> > > BTW I just tested and see something odd about the ``<addons></addons>`` case:
> > > 
> > > * if we have not applied any updates, nothing happens and this is logged:
> > > https://dxr.mozilla.org/mozilla-central/rev/
> > > c9a70b64f2faa264296f0cc90d68a2ee2bac6ac5/toolkit/mozapps/extensions/internal/
> > > XPIProvider.jsm#3045
> > 
> > Ah, that makes sense, the two empty sets match. Reading through the code
> > again, `addonList` is `null` if the `<addons>` tag is missing, and `[]` if
> > it is present but empty.
> >  
> > > * if there *have* been any updates applied, then instead all updates are
> > > removed and all default add-ons are disabled, as documented.
> > > 
> > > Based on the log messages, it's making it all the way to the end of
> > > `updateSystemAddons()` in this case.
> > 
> > I guess I don't quite understand what
> > `systemAddonLocation.cleanDirectories()` is actually doing. Is it literally
> > deleting all the system add-ons from the profile directory? I expected it to
> > just get rid of temporary directories or something.
> 
> 
> It removes outdated sets of update add-ons - each set of add-ons are
> installed into a subdirectory generated with a UUID:
> http://searchfox.org/mozilla-central/rev/
> 92616e983b8ad6e99ec148f341e146c3c6fa312a/toolkit/mozapps/extensions/internal/
> XPIProvider.jsm#8395
> 
> `cleanDirectories()` removes files not currently in use or pending use after
> restart.
> 
> Temporary files go in the OS-specified temp locations so they aren't really
> an issue.
> 
> 
> > > Fixing this should probably be a separate bug, but should we document the
> > > current behavior for now or leave it alone?
> > 
> > Documenting the current behavior I think is the thing to do.
> 
> 
> OK will do, I think it's definitely confusing as-is and something we should
> fix but I'll file a separate bug for that.

Filed bug 1287191 to follow up on this.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b1f7a028b0e

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/97b065bd2fba