633433 - If I chose Clear History when Minefield closes, Firefox would not start after quit browser immediately

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Alice0775 White]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110210 Firefox/4.0b12pre ID:20110210030400

If chose "Clear history when Minefield closes", Firefox would not start after quit browser immediately.
An aleart pops up.
 "Minefield cannot use the profile "xxx" because it is in use."

This problem does not happens if I un-checked "Clear history when Minefield closes"

Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile + several prug-ins + but no addons
2. Enabled "Clear history when Minefield closes" 
   (Tool > Options > Privacy > Choose "Use custom settings for history" 
    and Check "Clear history when Minefield closes" )
3. Exit Browser (File > Exit)
4, Start Browser immediately

Actual Results:
 An aleart pops up.
 "Minefield cannot use the profile "xxx" because it is in use."

Expected Results:
 In new profile and no addon, the browser should start immediately.

Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/db3e06f7018e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110208 Firefox/4.0b12pre ID:20110208133732
Fails:
http://hg.mozilla.org/mozilla-central/rev/37094ed97c9e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110208 Firefox/4.0b12pre ID:20110208142057
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=db3e06f7018e&tochange=37094ed97c9e

Comment 1

[Dietrich Ayala (:dietrich)]

There was a ton of work on Places to fix delayed shutdown for this release, and this regression might roll it all back.

Hard blocking, because of all the problems that delayed shutdown cause (slow addon restarts, updates are even more painful, etc).

Comment 2

[[e-mail]]

I'm simply not going to have time to work on this in time for release. :( Assigning to Josh for now so he can figure out what to do with it.

OTOH I'm not sure why this should happen. Shutdown should block until all this work is done, right? (Which, I agree, is a problem in itself, but let's deal with one thing at a time.)

Using this feature at shutdown automatically implies some kind of penalty, and we have to decide how much is tolerable. As for improving it, I see a few options:

1) Don't call CPD on plugins that haven't been instantiated during the browser session. This seems like a pretty simple one. It means that plugin data accrued outside the browser (e.g. via another browser) won't get cleared, but that seems reasonable to me.

2) Instantiate the plugins during idle-ish points so they're available at shutdown. This could be done in conjunction with 1) and would certainly lower the cost, but wouldn't prevent plugins blocking shutdown for e.g. I/O.

3) Make the NPAPI call asynchronous. This implies that the plugin-container would have to be able to live past the lifetime of the browser, by definition. That seems pretty infeasible to me.

4) Get plugins to cheapen their implementation of CPD by doing the work lazily.

We currently block shutdown on terminating the plugin-container processes without prejudice, right?

Comment 3

[Benjamin Smedberg]

It seems that what's going on here is that we're instantiating every plugin at shutdown. This is arguably what the user asked for, but launching a bunch of plugin-containers is obviously going to hurt shutdown perf.

Comment 4

[Josh Aas]

Attachment: fix v1.0

This should work but I haven't heavily tested it yet, so not ready for review. Feel free to comment on the strategy though.

Comment 5

[[e-mail]]

Comment on attachment 513189 [details] [diff] [review]
fix v1.0

> NS_IMETHODIMP
> nsPluginHost::ClearSiteData(nsIPluginTag* plugin, const nsACString& domain,
>                             PRUint64 flags, PRInt64 maxAge)
> {
>   // maxAge must be either a nonnegative integer or -1.
>   NS_ENSURE_ARG(maxAge >= 0 || maxAge == -1);
> 
>-  nsPluginTag* tag = EnsurePlugin(plugin);
>-  if (!tag) {
>+  nsPluginTag* tag = static_cast<nsPluginTag*>(plugin);

Hmm. What if the nsIPluginTag is a reference held onto by something (e.g. JS code) but the underlying plugin gets destroyed, or that plugintag gets otherwise removed from nsPluginHost's list, before someone calls CSD on it? I'm not sure if that can happen in practice. That's the reason I chose to iterate the pluginhost's list.

>+  // We only support clearing Flash site data for now.
>+  if (!tag->mIsFlashPlugin) {
>+    return NS_ERROR_FAILURE;
>+  }

Heh. I guess this is OK for now, and if other plugins implement it we can update this code in a point release.

But is it really worth it? If we implement my strategy in 1), we'd only do this for plugins that are already instantiated, and for those that don't support it the cost will be basically zero. (A sync call isn't even involved since we cache the flag on the parent.) And for those that do support it we definitely want to make the call. :)

Comment 6

[Josh Aas]

Attachment: fix v1.1

Still not ready for review but contains two improvements.

1) Also allow acting on plugins that are already loaded in addition to Flash.
2) Also restrict "SiteHasData" otherwise it can load plugins that aren't already loaded that are not Flash.

Comment 7

[Josh Aas]

Attachment: fix v1.2

Comment 8

[[e-mail]]

Comment on attachment 513416 [details] [diff] [review]
fix v1.2

Looks OK. What's your rationale for explicitly instantiating flash? I don't really think that's necessary, but won't hold review on it.

Comment 9

[Josh Aas]

We want the system to behave as intended for the Flash plugin because of its popularity and Adobe's schedule for supporting this feature. This way, clearing data will always clear data for Flash.

Comment 10

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/328483e1b820

Marking this as fixed since this patch should drastically reduce the potential work load on shutdown. If we still have a problem let us know.

Comment 11

[AndreiD[QA]]

Works For Me on:
Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110220 Firefox/4.0b12pre

I did a timing for the shut down, and for the build above got 5.6 seconds vs. 9.3 seconds for the build in the description (10 February).