713172 - Download Manager breaks shutdown sanitization, forcing sanitization at startup instead

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Ziga Seilnacht]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20111222 Firefox/12.0a1
Build ID: 20111222031055

Steps to reproduce:

1. Install Nightly version 12.0a1; the regression happened on the same day as the version bump from 11 to 12.
2. Start Nightly with a fresh profile.
3. Open preferences, privacy tab. Select Nightly will: Use custom settings for history.
4. Check "Clear history when Nightly closes", then click the settings button for this option.
5. Select only "When I quit Nightly, it should automatically clear all: Cookies".
6. Restart Nightly. 


Actual results:

The Flash plugin was loaded on browser startup (plugin-container.exe is running immediately after browser startup, even when using about:blank for homepage).


Expected results:

The Flash plugin should be instructed to clear LSOs at browser shutdown, not at startup, and only if the plugin has been loaded in the current browser session.

The pushlog for the regression is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2afd7ae68e8b&tochange=cd921d073b22

The most likely cause of the observed change would seem to be:
http://hg.mozilla.org/mozilla-central/rev/b121a045b451

The comments in bug 709262 don't mention Flash or cookies, so I suspect this change was unintentional.

Comment 1

[Tim (fmdeveloper)]

Josh -> Can you take a quick look and see if this might be related to bug 709262?

Comment 2

[Josh Matthews [:jdm]]

Thank you for this report Ziga, this is a great find. It turns out it's not bug 709262 at fault, but bug 711536 instead. We close the download connection early enough in the shutdown process that when we try to check the download manager (to determine if we even _can_ sanitize it), we throw an error, even though we're not trying to clear the downloads.

Comment 3

[Josh Matthews [:jdm]]

Attachment: Part 1: Avoid aborting the whole sanitization if the download manager isn't available.

Comment 4

[Josh Matthews [:jdm]]

Attachment: Part 2: Ensure the download manager database connection isn't closed while it could still be required.

Comment 5

[Josh Matthews [:jdm]]

Comment on attachment 584226 [details] [diff] [review]
Part 1: Avoid aborting the whole sanitization if the download manager isn't available.

Gavin, I'm undecided on whether we want this or not. It will stop a misbehaving routine from aborting the sanitization unnecessarily in the future, but it will also hide similar problems that crop up.

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 584227 [details] [diff] [review]
Part 2: Ensure the download manager database connection isn't closed while it could still be required.

Is there any reason the connection needs to be closed during quit-application instead of later?

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 584226 [details] [diff] [review]
Part 1: Avoid aborting the whole sanitization if the download manager isn't available.

Seems to me like nsDownloadManager::GetCanCleanUp should just never fail (and default to returning false).

Comment 8

[Marco Bonardo [:mak]]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #7)
> Seems to me like nsDownloadManager::GetCanCleanUp should just never fail
> (and default to returning false).

I agree, this is an attribute, should not throw, the error should be handled internally.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 584227 [details] [diff] [review]
Part 2: Ensure the download manager database connection isn't closed while it could still be required.

Review of attachment 584227 [details] [diff] [review]:
-----------------------------------------------------------------

We likely want a test for this.  It should ensure the sanitize process is not interrupted, maybe it could fire a topic when all items are done, and the test may ensure that it fires properly when it is invoked in the shutdown path

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +1942,5 @@
> +    // Null statements to finalize them.
> +    mGetIdsForURIStatement = nsnull;
> +    mUpdateDownloadStatement = nsnull;
> +    mozilla::DebugOnly<nsresult> rv = mDBConn->Close();
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

(In reply to Josh Matthews [:jdm] from comment #6)
> Is there any reason the connection needs to be closed during
> quit-application instead of later?

not specifically, but xpcom-shutdown is wrong, the later point you can use is profile-before-change, that is likely what we should use here.

Comment 10

[Josh Matthews [:jdm]]

Marco, I'm unsure of how to write a test that cares about actions during shutdown. Do you have any suggestions?

Comment 11

[Josh Matthews [:jdm]]

Attachment: Part 2: Ensure the download manager database connection isn't closed while it could still be required.

Comment 12

[Marco Bonardo [:mak]]

(In reply to Josh Matthews [:jdm] from comment #10)
> Marco, I'm unsure of how to write a test that cares about actions during
> shutdown. Do you have any suggestions?

hm, interesting problem, note that considered the bugs caused by this regression we may even follow-up with the test, since not having a fix is worse.
I made this test in the past that does something similar, making another test based on it that involves downloads may be possible.
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/tests/unit/test_clearHistory_shutdown.js

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 586854 [details] [diff] [review]
Part 2: Ensure the download manager database connection isn't closed while it could still be required.

Review of attachment 586854 [details] [diff] [review]:
-----------------------------------------------------------------

In some rare case, where async notifications may arrive later than profile-before-change, this may not yet be perfect (xpcom-shutdown would not be fine as well, async things may go further, bug 687726 covers that), but making it correct is more complicated, we should probably spin the async events loop in Places shutdown to ensure all notifications are fired earlier. But this is rather for a follow-up bug.

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +899,4 @@
>    (void)mObserverService->AddObserver(this, NS_IOSERVICE_GOING_OFFLINE_TOPIC, false);
>    (void)mObserverService->AddObserver(this, NS_IOSERVICE_OFFLINE_STATUS_TOPIC, false);
>    (void)mObserverService->AddObserver(this, NS_PRIVATE_BROWSING_REQUEST_TOPIC, false);
>    (void)mObserverService->AddObserver(this, NS_PRIVATE_BROWSING_SWITCH_TOPIC, false);

interesting, we never remove these observers, I wonder if there's a bug filed anywhere. On the other side this may use weak references, being a service. Please file a bug for this if one doesn't exist yet.

@@ +901,5 @@
>    (void)mObserverService->AddObserver(this, NS_PRIVATE_BROWSING_REQUEST_TOPIC, false);
>    (void)mObserverService->AddObserver(this, NS_PRIVATE_BROWSING_SWITCH_TOPIC, false);
>  
>    if (history)
>      (void)history->AddObserver(this, false);

As well as this one.

@@ +1944,5 @@
>        return CancelDownload(id);
> +  } else if (strcmp(aTopic, "profile-before-change") == 0) {
> +    // Null statements to finalize them.
> +    mGetIdsForURIStatement = nsnull;
> +    mUpdateDownloadStatement = nsnull;

Since bug 713140, let's ->Finalize() instead of nullifying, that will assert if some code tries to use them after, but at least shouldn't crash on dereferencing.

Comment 14

[Josh Matthews [:jdm]]

(In reply to Marco Bonardo [:mak] from comment #13)
> ::: toolkit/components/downloads/nsDownloadManager.cpp
> @@ +899,4 @@
> >    (void)mObserverService->AddObserver(this, NS_IOSERVICE_GOING_OFFLINE_TOPIC, false);
> >    (void)mObserverService->AddObserver(this, NS_IOSERVICE_OFFLINE_STATUS_TOPIC, false);
> >    (void)mObserverService->AddObserver(this, NS_PRIVATE_BROWSING_REQUEST_TOPIC, false);
> >    (void)mObserverService->AddObserver(this, NS_PRIVATE_BROWSING_SWITCH_TOPIC, false);
> 
> interesting, we never remove these observers, I wonder if there's a bug
> filed anywhere. On the other side this may use weak references, being a
> service. Please file a bug for this if one doesn't exist yet.

According to http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#890, removing them is unneeded. Is it still worth filing a bug?

Comment 15

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a1a6f5452614

Comment 16

[Josh Matthews [:jdm]]

Filed bug 716603 for the test.

Comment 17

[Marco Bonardo [:mak]]

(In reply to Josh Matthews [:jdm] from comment #14)
> According to
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/
> nsDownloadManager.cpp#890, removing them is unneeded. Is it still worth
> filing a bug?

Well, the comment is a bit nonsense, the fact download manager is a singleton service just means it may use weak references, not that it shouldn't care about keeping others alive. Doing the cleanup or using wek refs may simplify the shutdown work for the cycle collector.

Comment 18

[Marco Bonardo [:mak]]

and once this lands in central, we should get approval for Aurora

Comment 19

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/a1a6f5452614

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 586854 [details] [diff] [review]
Part 2: Ensure the download manager database connection isn't closed while it could still be required.

[Approval Request Comment]
Regression caused by (bug #): bug 711536
User impact if declined: Clear private data on shutdown is broken and may crash (bug 713140)
Testing completed (on m-c, etc.): m-c, filed bug to investigate an automated test
Risk to taking this patch (and alternatives if risky): low impact

Comment 21

[Marco Bonardo [:mak]]

you didn't address my third comment, thus looks like we still crash.

Comment 22

[Marco Bonardo [:mak]]

Comment on attachment 586854 [details] [diff] [review]
Part 2: Ensure the download manager database connection isn't closed while it could still be required.

will need a coalesced patch for approval

Comment 23

[Josh Matthews [:jdm]]

Weird, I definitely made the change locally. Not quite sure what happened.

Comment 24

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5a020d632a28

Comment 25

[Josh Matthews [:jdm]]

Attachment: Part 2: Ensure the download manager database connection isn't closed while it could still be required.

Comment 26

[Josh Matthews [:jdm]]

Attachment: Ensure the download manager database connection isn't closed while it could still be required.

Actually updated. Weirdness!

Comment 27

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/5a020d632a28

Comment 28

[Josh Matthews [:jdm]]

Comment on attachment 587827 [details] [diff] [review]
Ensure the download manager database connection isn't closed while it could still be required.

[Approval Request Comment]
Regression caused by (bug #): bug 711536
User impact if declined: Clear private data on shutdown is broken and may crash (bug 713140)
Testing completed (on m-c, etc.): m-c, filed bug to investigate an automated test
Risk to taking this patch (and alternatives if risky): low impact

Comment 29

[Scoobidiver (away)]

(In reply to Josh Matthews [:jdm] from comment #28)
> User impact if declined: Clear private data on shutdown is broken and may
> crash (bug 713140)
bug 713140 is #1 top crasher in Aurora.

Comment 30

[Marco Bonardo [:mak]]

also crash bug 714627 is going to be solved by this, according to crash-stats crashes by buildid (and the stack confirms a null dereference on a statement)

Comment 31

[Matt Brubeck (:mbrubeck)]

This landed on Aurora but was backed out:
https://hg.mozilla.org/releases/mozilla-aurora/rev/16b8c84705a4
https://hg.mozilla.org/releases/mozilla-aurora/rev/3b3a05acd51d

because of crashes in xpcshell tests:
https://tbpl.mozilla.org/php/getParsedLog.php?id=8622125&tree=Mozilla-Aurora
https://tbpl.mozilla.org/php/getParsedLog.php?id=8622493&tree=Mozilla-Aurora

Comment 32

[Josh Matthews [:jdm]]

This bounced because the patch from bug 713242 is not present on mozilla-aurora. I am having trouble figuring out why we're not seeing similar failures since bug 711536 was uplifted? Marco, any ideas?

Comment 33

[Marco Bonardo [:mak]]

I think the problem is the
mozilla::DebugOnly<nsresult> rv = mDBConn->Close();
MOZ_ASSERT(NS_SUCCEEDED(rv));

when this patch landed in central the test was already correctly finalizing statements. When the original patch landed in central the test was not sending quit-application so that code was not invoked at all. When Rafael made the test fire quit-application he found the assertion and finalized the statements.

Since we don't know if there may be other tests in need of finalization after this one (well, we may use tryserver, but it's more time consuming), I think the safer way out of this is just to backout bug 711536 from Aurora, there are no negatives in doing that.

Comment 34

[Josh Matthews [:jdm]]

Backed out bug 711536 in http://hg.mozilla.org/releases/mozilla-aurora/rev/cd135c7ca5af. This bug should now be resolved in all channels.

Comment 35

[Justin Wood (:Callek)]

Comment on attachment 587827 [details] [diff] [review]
Ensure the download manager database connection isn't closed while it could still be required.

clearing a+ as this patch was backed out for problems, and the bug was resolved by backing out another bug.