Re: Declarative rules should be removed on uninstalling, not unloading (issue 52743002)

[va...@chromium.org]

Hi Jeffrey,

I'd be grateful for your opinion on the following:

RulesRegistry needs to distinguish between install/uninstall a
enable/disable
changes for extensions.
For install, it needs to perform AddRules, including AddRulesImpl, and
similarly
for uninstall and RemoveRules.
For enable it only needs to perform AddRulesImpl, and similarly
RemoveRulesImpl
for disable.

In terms of NOTIFICATION_EXTENSION_INSTALLED,
NOTIFICATION_EXTENSION_UNINSTALLED, NOTIFICATION_EXTENSION_LOADED, and
NOTIFICATION_EXTENSION_UNLOADED events (I'll drop the common prefix below),
the
following happens:
on install: INSTALLED, then LOADED
on uninstall: UNLOADED, then UNINSTALLED
on enabled: LOADED
on disabled: UNLOADED
So the most natural seems to be responding with AddRulesImpl to LOADED,
RemoveRulesImple to UNLOADED, AddRules (without calling *Impl) to INSTALLED
and
RemoveRules (without calling *Impl) to UNINSTALLED.

However, there is a problem with reloading extensions.
ExtensionService::ReloadExtension causes this sequence: UNLOADED →
INSTALLED →
LOADED. This misses UNINSTALLED, which would result in duplicating the
rules in
RulesRegistry.
I tried to understand whether this omission is by design or not, but I
could not
convince myself either way.

My favourite option so far would be erasing all rules stored for an
extension in
RulesRegistry as soon as new are being saved due to the INSTALLED
notification.
Obviously if ExtensionService::ReloadExtension should actually emit
UNINSTALLED,
then my preferred solution changes to fixing that first.
I'm also open to any other suggestions, if you have any.

Thanks, and looking forward to hearing from you :)
Vaclav

https://codereview.chromium.org/52743002/

[Jeffrey Yasskin]

On Wed, Nov 6, 2013 at 8:00 AM, <va...@chromium.org> wrote:
> Hi Jeffrey,
>
> I'd be grateful for your opinion on the following:
>
> RulesRegistry needs to distinguish between install/uninstall a
> enable/disable
> changes for extensions.
> For install, it needs to perform AddRules, including AddRulesImpl, and
> similarly
> for uninstall and RemoveRules.
> For enable it only needs to perform AddRulesImpl, and similarly
> RemoveRulesImpl
> for disable.
>
> In terms of NOTIFICATION_EXTENSION_INSTALLED,
> NOTIFICATION_EXTENSION_UNINSTALLED, NOTIFICATION_EXTENSION_LOADED, and
> NOTIFICATION_EXTENSION_UNLOADED events (I'll drop the common prefix below),
> the
> following happens:
> on install: INSTALLED, then LOADED
> on uninstall: UNLOADED, then UNINSTALLED
> on enabled: LOADED
> on disabled: UNLOADED
> So the most natural seems to be responding with AddRulesImpl to LOADED,
> RemoveRulesImple to UNLOADED,
> AddRules (without calling *Impl) to INSTALLED

Note that there aren't any rules to add on the INSTALLED event, since
the extension hasn't added them yet.

> and
> RemoveRules (without calling *Impl) to UNINSTALLED.

Sounds good.

> However, there is a problem with reloading extensions.
> ExtensionService::ReloadExtension causes this sequence: UNLOADED → INSTALLED
> →
> LOADED. This misses UNINSTALLED, which would result in duplicating the rules
> in
> RulesRegistry.
> I tried to understand whether this omission is by design or not, but I could
> not
> convince myself either way.

Argh. ReloadExtension is the bane of my existence. ReloadExtension
also causes DISABLE events sometimes, with DISABLE_RELOAD set
(https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/extensions/extension.h&l=86),
and I think this lasts through the next install. Check
extension_prefs_->GetDisableReasons(extension_id) &
Extension::DISABLE_RELOAD.

> My favourite option so far would be erasing all rules stored for an
> extension in
> RulesRegistry as soon as new are being saved due to the INSTALLED
> notification.

I think this doesn't quite work because extension upgrades shouldn't
cause rules to get dropped, but I think they do cause the installed

notification.

> Obviously if ExtensionService::ReloadExtension should actually emit
> UNINSTALLED,
> then my preferred solution changes to fixing that first.
> I'm also open to any other suggestions, if you have any.
>
> Thanks, and looking forward to hearing from you :)
> Vaclav
>
>
> https://codereview.chromium.org/52743002/
>

> --
> You received this message because you are subscribed to the Google Groups
> "Extensions reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to extensions-revi...@chromium.org.
> To view this discussion on the web visit
> https://groups.google.com/a/chromium.org/d/msgid/extensions-reviews/089e0158bb46a743c404ea8440b9%40google.com.

[fsa...@chromium.org]

This patch is now stale after my landing some refactorings to RulesRegistry.

https://codereview.chromium.org/52743002/

[va...@chromium.org]

On 2013/11/07 18:23:07, Fady Samuel wrote:
> This patch is now stale after my landing some refactorings to
> RulesRegistry.

Yeah, I was prepared for that, thanks for heads-up.
Will adapt to the new code and update this soon.

https://codereview.chromium.org/52743002/