How to handle runtime.reload misuse

[Marijn Kruisselbrink]

I'm currently working on changing chrome.runtime.reload to make it harder for an extension to accidentally get stuck in a reload loop, if because of some bug the extension ends up calling reload while being loaded. Detecting when this situation occurs is pretty easy, but I'm not entirely sure what the right behavior should be when this situation is detected.

One option would be to report an error the same way any other extension api reports an error, but that would mean extensions would have to pass a callback to reload only for this unlikely error situation.

On the other extreme end it would be an option to disable the extension completely similarly as when the renderer process crashes. In addition some UI (a similar crash bubble but with a different text) should probably be shown.

And a third option might be to just print an error to the console of the page calling reload (while refusing to reload).

I originally thought the option of forcibly disabling the extension would be the most sensible way to go, but I'm not entirely sure of that anymore (and if that is the solution I go for, should it only trigger on runtime.reload calls, or should it take into account any method for an extension to be reloaded?).

Marijn

[Mike Tsao]

I'd disable. Calling reload on oneself is always going to be an outlier case, so it's reasonable to expect a developer to take special care to handle it correctly. Better to fail fast and fail loudly.

--
You received this message because you are subscribed to the Google Groups "extensions-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to extensions-de...@chromium.org.
For more options, visit https://groups.google.com/a/chromium.org/groups/opt_out.

[Mike Tsao]

> Be sure to put a descriptive error message somewhere users can find it. This is going to get
> past some developers, and we want users to be able to explain what went wrong instead of
> thinking chrome is flakily disabling random extensions.

Yes. Our crash reporting for developers is not exactly fantastic, so a
custom UI message (subject to the telephone effect from their users
saying "it said something about looping") would at least help
developers narrow down the cause.

[Marijn Kruisselbrink]

On Mon, Jan 6, 2014 at 9:13 AM, Joe Marini <joem...@google.com> wrote:

Just curious, do we have any data on how often this is happening?

I'm not sure how often this is happening, but we've had at least one hard to debug case of an extension that ran into this problem (http://crbug.com/329322).

Yes, the text has to fairly clear to users, since for a large part this is also to protect the user from badly behaving extensions. I think I'll just reuse the same bubble that is used when an extension is disabled because a renderer crashed, but with a different text.

[David Levin]

What does it mean to call reload while being loaded? (When would the loaded state be done?)

How can an extension tell if it is an ok time to call reload (so that it won't get disabled)?

dave

To unsubscribe from this group and stop receiving emails from it, send an email to apps-dev+u...@chromium.org.

[Mike Tsao]

I'm sure Marijn has figured out a better heuristic, but I'd say if you manage a few idle event loops or two without calling reload, then you're probably out of your initialization code, so the system can reset whatever watchdog counter it's using.

[Marijn Kruisselbrink]

First of all, regardless of how you defined "while being loaded", calling reload the first couple of times will still work just fine. It doesn't seem too unreasonable for an extension to sometimes want to reload itself just after it got loaded. Only when an extension repeatedly tries to reload itself too quickly will it get disabled. (currently repeatedly is defined as > 5 times in a row, and too fast as within 5 seconds of the previous call to .reload). It probably makes sense to also add a UMA histogram to keep track of how often this will result in an extension being disabled.

And finally .reload will still be allowed to go through if reloading the extension would result in a pending update being applied, although it seems like it would be very rare for an extension to reload itself just often enough so it would be disabled on the next reload, but a new version just happens to be downloaded just before that next reload happens. So maybe I shouldn't even bother with that extra exception.

[Mike Tsao]

> [snip] It probably makes sense to also add a UMA histogram to keep track
> of how often this will result in an extension being disabled. [snip]

Makes sense. Keep an eye on this and tweak the logic as needed.

[Marijn Kruisselbrink]

+bartfab (who seems to be working on a very similar feature where a force-installed extension that repeatedly reloads because the renderer process crashed gets disabled; for background, what I'm working on is disabling an extension when it repeatedly reloads because it calls reload while it is being loaded)

Looking at the code, it seems bartfab implemented a very similar feature a while back, but then later reverted it because the UI team had issues with the UI. Since the UI I was going for would be very similar (and the implementation/feature would be nearly identical too), what were the problems with the UI?

Also does anybody know why all the code that deals with extension crash bubbles lives in browser/background/backgroun_contents_service.h, which claims to only be responsible for managing BackgroundContents instances and keeping the browser process alive in certain cases? It doesn't seem like crashing extensions have anything to do with background behavior...

[David Levin]

Slightly off topic, it would be awesome if crashing once didn't disable an extension. There are cases where an extension may be triggering the crash (due to doing just the right combination of things or consuming excessive memory) but the typical case that I've seen is due to some one off Chrome bug that randomly occurred.

dave

To unsubscribe from this group and stop receiving emails from it, send an email to apps-dev+u...@chromium.org.

[David Levin]

[Bartosz Fabianowski]

+ainslie, +finnur

Yes, UI did not like the current bubble being reused. There is a
proposal blessed by UI on the table now. This was proposed by Alex and
involves reusing a bubble that Finnur is currently implementing.

- Bartosz

[Finnur Thorarinsson]

I don't remember seeing that proposal from Alex, but just before Christmas I added an extension bubble framework that makes it easy to pop up a bubble to call attention to certain extensions and let the user know why they are being singled out. I added two bubbles using that framework: The Proteus Wipeout bubble (showing which extensions have been disabled) and the DevMode bubble (showing which extensions are running in dev mode, i.e. from a command line/unpacked). Those two bubbles are checked in, so (apart from some cleanup) there's no bubble I'm currently implementing.

[Bartosz Fabianowski]

Yes, that is the bubble Alex was referring to. I guess it was still
in-progress back then. FTR, Alex' proposal is in a thread titled "String
review required for 'enterprise extension misbehaving' notification" -
Finnur, you were CC'd on that thread.

- Bartosz

[Finnur Thorarinsson]

Ah... That thread. Thank you.

It didn't mention the 'reload' issue so I wasn't able to connect the dots (I'm also behind on email, as I was on paternal leave). :)

[Marijn Kruisselbrink]

On Tue, Jan 7, 2014 at 4:47 AM, Finnur Thorarinsson <fin...@chromium.org> wrote:

I don't remember seeing that proposal from Alex, but just before Christmas I added an extension bubble framework that makes it easy to pop up a bubble to call attention to certain extensions and let the user know why they are being singled out. I added two bubbles using that framework: The Proteus Wipeout bubble (showing which extensions have been disabled) and the DevMode bubble (showing which extensions are running in dev mode, i.e. from a command line/unpacked). Those two bubbles are checked in, so (apart from some cleanup) there's no bubble I'm currently implementing.

Is that extensions::ExtensionMessageBubbleController? I'm having some trouble figuring out how I would use that framework to pop-up a "this extension got disabled for misbehaving" bubble. Is there some documentation anywhere about this framework, the few comments the class has don't really help explain how it is supposed to be used (and it isn't quite clear which methods are supposed to be public API and which aren't, since all its members are currently public.

And just to make sure, is this bubble is implemented on all platforms? I only see a Views implementation, so that would mean this bubble doesn't yet work on Mac (and non-aura linux)?

[Finnur Thorarinsson]

>  I only see a Views implementation, so that would mean this bubble doesn't yet work on Mac (and non-aura linux)?

Correct. This has not been made cross-platform because the two bubbles we needed to show were Windows only. There's no real reason not to add support for other platforms, though, if you have that need. Thanks to Kalman's twisting of my arm :) during review the amount of Views-specific code to port has been greatly reduced (see ExtensionMessageBubbleView).

To see one of my bubbles in action, start Chrome with --enable-force-dev-mode-highlighting, load an unpacked extension and restart Chrome. The bubble appears on startup explaining that your unpacked extension is in dev mode. The controller for that class is the DevModeBubbleController and you can set breakpoints in that class and probably pretty much mimic what that does.

But the gist of what you need to do is create an instance of your BubbleController class and call ShouldShow on it (for an example, see ExtensionMessageBubbleView::MaybeShow). That will trigger a chain of events that leads to the ExtensionMessageBubbleController::Delegate methods being called on your class. One of those functions is ShouldIncludeExtension(id) (where id is the id of the extension you want called out -- for crashing, in your case). If you return true from that function then ShouldShow will also return true and you should as a result create a ExtensionMessageBubbleView object, pass your controller to it and call Show() on the controller. This will lead to more of your Delegate methods being called to determine the contents of the Bubble (text and link destination, etc). Again, see MaybeShow for an example.

Note: Both currently implemented types of bubbles run at startup and they reflect something that has already happened that can be queried (e.g. a set of extensions have been wiped). Judging by your comments, you are looking to trigger this for one extension as it crashes. I wonder, though, what happens if two or more extensions crash at close proximity in time to each other? You are most likely to see these crashes at startup, right? Should you not keep track of when the first extension crashes, wait a bit and then display "the following extensions were disabled because they crashed" (listing any other extension that also crashed while you waited)? Just a thought. Then you have essentially exactly the same flow as the other two bubbles and the framework is a direct fit.

If not, there's still a way to implement the controller class in such a way that only one extension is shown. But lets cross that bridge when we get to it.

[Bartosz Fabianowski]

There are two cases in which we would like to reuse your bubble:

* Marijn wants to shown bubbles for extensions that reload themselves
during load, causing a loop.
* I want to show bubbles for force-installed extensions that get stuck
in a crash loop.

The UI requirements for both cases should be extremely similar. I
discussed all the corner cases I could think of (such as multiple
extensions crashing) with the UI team. The flow that we agreed on is
documented here:

http://crbug.com/175701#c16

I think it would be best if the same UI flow was used in Marijn's and my
work for consistency.

On 01/08/2014 12:39 PM, Finnur Thorarinsson wrote:
>> I only see a Views implementation, so that would mean this bubble
> doesn't yet work on Mac (and non-aura linux)?
>
> Correct. This has not been made cross-platform because the two bubbles we

> needed to show were Windows only. There's no real reason *not* to add

[Finnur Thorarinsson]

Hmm... I did not expect the bubble scene to get so crowded so quickly... :/

TL;DR Both of these use cases are better implemented as labels on the Extensions page and not as bubbles. 

Slightly more detailed: I think we need to take a step back here...

I would argue we should do something else for Marijn's case (a bubble is way too drastic a measure). A reload loop is an internal error in the extension that I don't think the user needs to know about, probably does not understand and almost certainly does not want to be bothered about. Those who do care will go to HotDog \ Tools \ Extensions to figure out if their extension is gone, so we should show a message there saying: "disabled because of an internal error in the extension" or something. Another way to think about it is this: What is the user expected to do when they see the bubble? Is there anything actionable there? They're not going to go debug/fix the extension. At most, they'd uninstall it. But, we've already disabled the extension, so uninstalling it isn't really necessary. I don't see the benefit of being in the user's face with this and I think it would go a little bit against our core Chrome UX principles.

The force-installed extensions matter is a little more complicated because we have no way of contacting the person who arguably needs to know (the admin). But we have to ask ourselves here: Yes, the bubble might result in the admin getting to know about the problem through user complaints -- but is making the user suffer really the right thing to do here? I don't think it is our job to make Chrome annoying to use, just so that the admin can fix a problem with some random extension they force-installed.

I guess it boils down to this: Can anyone explain to me how it helps Chrome to take an internal error in an extension and making it the user's problem?

[Bartosz Fabianowski]

On 01/08/2014 03:51 PM, Finnur Thorarinsson wrote:
> Hmm... I did not expect the bubble scene to get so crowded so quickly... :/
>
> TL;DR Both of these use cases are better implemented as labels on the
> Extensions page and not as bubbles.
>
> Slightly more detailed: I think we need to take a step back here...
>
> I would argue we should do something else for Marijn's case (a bubble is
> way too drastic a measure). A reload loop is an internal error in the
> extension that I don't think the user needs to know about, probably does
> not understand and almost certainly does not want to be bothered about.

> Those who *do* care will go to HotDog \ Tools \ Extensions to figure out if

> their extension is gone, so we should show a message there saying:
> "disabled because of an internal error in the extension" or something.
> Another way to think about it is this: What is the user expected to do when
> they see the bubble? Is there anything actionable there? They're not going
> to go debug/fix the extension. At most, they'd uninstall it. But, we've
> already disabled the extension, so uninstalling it isn't really necessary.
> I don't see the benefit of being in the user's face with this and I think
> it would go a little bit against our core Chrome UX principles.
>
> The force-installed extensions matter is a little more complicated because
> we have no way of contacting the person who arguably needs to know (the
> admin). But we have to ask ourselves here: Yes, the bubble might result in
> the admin getting to know about the problem through user complaints -- but

> is making the user suffer *really* the right thing to do here? I don't

> think it is our job to make Chrome annoying to use, just so that the admin
> can fix a problem with some random extension they force-installed.

The user is already suffering: Because force-installed extensions are
used to perform important functions (e.g. ensure a corporate password
policy is followed), we never allow them to be disabled. If a
force-installed extension is crashing repeatedly, we keep restarting it
repeatedly, ad infinitum. This could degrade performance. Hence, the
damage is done already. We now want to tell the user why the computer is
getting sluggish and what they can do about it.

[Finnur Thorarinsson]

That doesn't apply to Marjin's case though.

[Bartosz Fabianowski]

On 01/08/2014 03:55 PM, Finnur Thorarinsson wrote:
> That doesn't apply to Marjin's case though.

Correct. My comment applies to force-installed extensions only.

[Finnur Thorarinsson]

I also doubt very much that an extension that is repeatedly being reloaded can effectively perform the important function it was meant for.

So why not disable it (regardless of whether we show a bubble or not)?

[Bartosz Fabianowski]

On 01/08/2014 03:59 PM, Finnur Thorarinsson wrote:
> I also doubt very much that an extension that is repeatedly being
> reloaded can effectively perform the important function it was meant for.
>
> So why not disable it (regardless of whether we show a bubble or not)?

We cannot tell whether the extension is crashing or is being crashed. A
user can use the OS task manager (or a shell loop) to repeatedly kill an
extension. Force-installed apps/extensions need to be restarted to avoid
this loophole.

[Finnur Thorarinsson]

I see.

For this new force-install-crash bubble we need to think about an additional concern that we need to address.

I'm going to maintain that the most likely time you'll see this new bubble is during startup when everything is firing up. We at least need to take that into account (that it might happen during startup) and think carefully about bubble priority to avoid having a bubble storm on startup, because that's an awful experience when you start up Chrome.

Currently (as you see from the code in MaybeShow) we only show one bubble at a time: DevMode is only shown if Wipeout is not. We can do that because normally Wipeout is only shown once (that is until you get new jerkware that we wipe out).

The problem is that with your new bubble we have two bubbles that will potentially show on every startup (yours and DevMode) and if we assign one higher priority than the other then the first will prevent the latter from showing at all. That means, we'll need code to serialize the showing of the bubbles (make sure only one is shown at a time).

Additionally, we might want to think about setting a limit for these bubbles, because after a long while I question whether there's much point in showing the bubble on every startup. But that's another issue altogether.

[Justin Dewitt]

Also does anybody know why all the code that deals with extension crash bubbles lives in browser/background/backgroun_contents_service.h, which claims to only be responsible for managing BackgroundContents instances and keeping the browser process alive in certain cases? It doesn't seem like crashing extensions have anything to do with background behavior...

At one point I knew the reason for this but I have since forgotten.  +atwilson should know though. 

Is there a reason you're not considering a notification for this purpose?  Many things that were formerly bubbles are moving to the notification center.

[Marijn Kruisselbrink]

On Wed, Jan 8, 2014 at 10:24 AM, Justin Dewitt <dew...@chromium.org> wrote:

Also does anybody know why all the code that deals with extension crash bubbles lives in browser/background/backgroun_contents_service.h, which claims to only be responsible for managing BackgroundContents instances and keeping the browser process alive in certain cases? It doesn't seem like crashing extensions have anything to do with background behavior...

At one point I knew the reason for this but I have since forgotten.  +atwilson should know though. 

Is there a reason you're not considering a notification for this purpose?  Many things that were formerly bubbles are moving to the notification center.

No reason for that, no, other than consistency. There are three different situations where an extension or app could get disabled because it misbehaved (crashed, or crashed too often too soon for a policy installed extension, or reloaded itself too often too soon), so it seemed sensible to me that all three of these situations should use a similar UI. The first one used a bubble, the second one was initially implemented to use the same bubble, so it seemed to make sense that the third situation should also use the same UI element. But yes, probably bubbles are not the right UI element anymore.

And as finnur pointed out, maybe my situation is differently enough that only showing it on the chrome://extensions page is enough, although if we have a way to non-intrusively notify a user that might still make sense. In the case of extensions it is probably fine to not have any immediate feedback, but if an app is misbehaving to cause it to get disabled this will most likely happen when trying to launch the app; I don't think it would be rather confusing if trying to launch an app would result in nothing (other than the app getting disabled).

[Drew Wilson]

On Wed, Jan 8, 2014 at 7:24 PM, Justin Dewitt <dew...@chromium.org> wrote:

Also does anybody know why all the code that deals with extension crash bubbles lives in browser/background/backgroun_contents_service.h, which claims to only be responsible for managing BackgroundContents instances and keeping the browser process alive in certain cases? It doesn't seem like crashing extensions have anything to do with background behavior...

Originally, the code in that file displayed a notification when an extension's background contents renderer process crashed, so the user could restart it. This was later extended (not by me) to display notifications for *all* crashing extensions - I allowed this code to be put in the same file against my better judgement because it was easier and allowed sharing code. I really regret allowing that latter change, because it's been a huge maintenance headache.

[Bartosz Fabianowski]

On 01/08/2014 04:17 PM, Finnur Thorarinsson wrote:
> I see.
>
> For this new force-install-crash bubble we need to think about an
> additional concern that we need to address.
>
> I'm going to maintain that the most likely time you'll see this new bubble
> is during startup when everything is firing up. We at least need to take
> that into account (that it might happen during startup) and think carefully
> about bubble priority to avoid having a bubble storm on startup, because
> that's an awful experience when you start up Chrome.

For force-installed extensions, we are planning to only pop up a bubble
if an extension crashes at least X number of times in a row. If a bubble
storm on start-up is a concern, we could further modify this so that the
bubble is shown no earlier than Y seconds after start-up. But we would
still have the issue of bubble priorities: If the dev mode bubble pops
up on start-up, the user does not dismiss it and the force-installed
crash bubble wants to pop up Y seconds later, what do we do?

Alex, you requested a bubble-based UI for crashing force-installed
extensions. Do you have any thoughts on how the conflict should be resolved?

[Finnur Thorarinsson]

I don't see how we can get away with not serializing the opening of bubbles.

I don't want to put the user in a situation where they are about to click a button in one bubble but as they click another bubble appears, which results in them taking a completely different action than they intended.

[Bartosz Fabianowski]

On 01/09/2014 12:01 PM, Finnur Thorarinsson wrote:
> I don't see how we can get away with not serializing the opening of bubbles.
>
> I don't want to put the user in a situation where they are about to click a
> button in one bubble but as they click another bubble appears, which
> results in them taking a completely different action than they intended.

ACK. Let's get Alex' feedback on this as he proposed and fleshed out the
bubble flow for crashing force-installed extensions.

[Finnur Thorarinsson]

On Thu, Jan 9, 2014 at 11:06 AM, Bartosz Fabianowski <bar...@chromium.org> wrote:

On 01/09/2014 12:01 PM, Finnur Thorarinsson wrote:

I don't see how we can get away with not serializing the opening of bubbles.

I don't want to put the user in a situation where they are about to click a
button in one bubble but as they click another bubble appears, which
results in them taking a completely different action than they intended.

ACK. Let's get Alex' feedback on this as he proposed and fleshed out the bubble flow for crashing force-installed extensions.

Alex?

Bartosz: please note that I refactored the bubble framework a little bit. Basically, I moved a part of the controller into a separate delegate class and changed the ownership model (the view now owns the controller and the controller owns the delegate).

https://codereview.chromium.org/134103002

For a new bubble, you can create your own classes based on DevModeBubbleController and DevModeBubbleDelegate.

We'll still need to sort out the possibility of a "bubble storm", though. In the short time since this framework was implemented three different people have come out of the woodwork with three different use cases for showing an extension message bubble. I don't think it will be long until we hit this problem.

[Bartosz Fabianowski]

On 01/17/2014 04:10 PM, Alex Ainslie wrote:
> +ruby, who is helping me think through some other bubble stuff (on the
> permissions side)
>
> Hmm. I'd also like to try to avoid the bubble-storn.
> "Ugh. Chrome, get out of my way!"
>
> I'm thinking that we should have some notion of priority baked in (on our
> side) and to only show one bubble. We're going to need this anyways because
> permissions should be shown instead of "feature" bubbles (ex: translate
> etc.) or errors.
>
> Do you think you could grab some time on my calendar?

I am not sure who the "you" is that was tasked with setting up a
meeting. Whoever "you" are, if you do set up a meeting, please add me as
well. I would like to participate in the discussion.

- Bartosz

> I'm worried that I've
> paged too much of this out over the holidays and would prefer to quickly
> chat in person.
>
> Alex

[Bartosz Fabianowski]

On 01/17/2014 06:22 PM, Alex Ainslie wrote:
> That "you" was you. :)

Ah :).

Done.

>
> I was imagining: bartfab, rubylee, saroop, finnur, ainslie
>
>
> On Fri, Jan 17, 2014 at 9:12 AM, Bartosz Fabianowski