428481 - nsFaviconService calls Abandon on a scoped global statement

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Marco Bonardo [:mak]]

Attachment: patch

This patch mitigate a possible problem with insert statement, we are calling Abandon that will nullify the statement but that appear not necessary for a global statement in a scoper. Fixed GetIcon to instantiate the service only if needed (perf). Removed double definition of nsIRequestObserver. Did some cleanup to make code more readable, should help finding problems.
*could* mitigate problems with wrong favicons, but we don't have clear STRs to reproduce that.

Comment 1

[Marco Bonardo [:mak]]

i suggest taking this for RC1 to see if favicons problems are reduced

Comment 2

[Marco Bonardo [:mak]]

asking blocking, i'm not sure if nullify the statement is our problem, still that appears quite wrong.

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 315067 [details] [diff] [review]
patch

r=me. nit: while you're there, please append "URI" to the parameter names that are nsIURIs.

also, please confirm that this code is already under test. if not, please add a test for it.

Comment 4

[Mike Connor [:mconnor]]

Not going to block on this, but we should take the patch.

Comment 5

[Mike Connor [:mconnor]]

Comment on attachment 315067 [details] [diff] [review]
patch

a=mconnor on behalf of 1.9 drivers

Comment 6

[Marco Bonardo [:mak]]

(In reply to comment #3)
> also, please confirm that this code is already under test.

that's interesting: 
- SetFaviconUrlForPage is not tested anywhere
- SetAndLoadFaviconForPage is tested for null params
i'll try to test db sanity of inserts for them since are the 2 methods that are using SetFaviconUrlForPageInternal

Comment 7

[Marco Bonardo [:mak]]

Attachment: wip

same patch, some unit test for faviconService API methods.
tests are PASS with or without the patch, i need to try getting a test that is showing the actual problem (not easy though).

i will try adding some test for a corrupt favicon, a no more existant one, and so on...

Comment 8

[Martijn Wargers (dead)]

Was the patch checked in?

Comment 9

[Marco Bonardo [:mak]]

(In reply to comment #8)
> Was the patch checked in?

no

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch

- changed method params to use URI suffix where applicable
- cleaned up some formatting (better code readability)
- removed unused get spec in lazymessage
- moved useless get faviconService in getIcon
- removed useless Abandon call
- added 4 new tests for the faviconService methods that were untested till now and cleaned up a bit favicon test file (checked all tests PASS)

this is probably unrelated to our favicon problems, i was not able to reproduce problems with a stress test (setting getting thousands of icons) nor to get a valid STR to reproduce that. Hwv nullify a statement could maybe interact with the GC, and new tests are welcome on this service.

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 319375 [details] [diff] [review]
patch

  page.
> 
> nsresult
>-nsFaviconService::UpdateBookmarkRedirectFavicon(nsIURI* aPage, nsIURI* aFavicon)
>+nsFaviconService::UpdateBookmarkRedirectFavicon(nsIURI* aPageURI,
>+                                                nsIURI* aFaviconURI)
> {
>-  NS_ENSURE_TRUE(aPage, NS_ERROR_INVALID_ARG);
>-  NS_ENSURE_TRUE(aFavicon, NS_ERROR_INVALID_ARG);
>+  NS_ENSURE_TRUE(aPageURI, NS_ERROR_INVALID_ARG);
>+  NS_ENSURE_TRUE(aFaviconURI, NS_ERROR_INVALID_ARG);

could use NS_ENSURE_ARG_POINTER, for a little more specificity.

r=me otherwise. thanks for the cleanup and tests.

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 319375 [details] [diff] [review]
patch

asking for new approval, more cleanup but also more tests

Comment 13

[Marco Bonardo [:mak]]

Attachment: for check-in

Comment 14

[Dietrich Ayala (:dietrich)]

drivers: this adds tests, and fixes a potential cause of the mismatched favicons some users are seeing.

the bulk of the patch is just cleanup to adhere to toolkit places style.

Comment 15

[Mike Schroepfer]

Comment on attachment 319748 [details] [diff] [review]
for check-in

a+ schrep based on Dietrich's risk assessment over IRC

Comment 16

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/public/nsIFaviconService.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsIFaviconService.idl,v  <--  nsIFaviconService.idl
new revision: 1.7; previous revision: 1.6
done
Checking in toolkit/components/places/src/nsFaviconService.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsFaviconService.cpp,v  <--  nsFaviconService.cpp
new revision: 1.25; previous revision: 1.24
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.305; previous revision: 1.304
done
Checking in toolkit/components/places/src/nsNavHistoryResult.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v  <--  nsNavHistoryResult.cpp
new revision: 1.146; previous revision: 1.145
done
Checking in toolkit/components/places/tests/unit/test_favicons.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_favicons.js,v  <--  test_favicons.js
new revision: 1.4; previous revision: 1.3
done

Comment 17

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv