Closed Bug 428481 Opened 12 years ago Closed 12 years ago

nsFaviconService calls Abandon on a scoped global statement

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 3

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #315067 - Flags: review?(dietrich)
i suggest taking this for RC1 to see if favicons problems are reduced
asking blocking, i'm not sure if nullify the statement is our problem, still that appears quite wrong.
Flags: blocking-firefox3?
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.
Attachment #315067 - Flags: review?(dietrich) → review+
Not going to block on this, but we should take the patch.
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Comment on attachment 315067 [details] [diff] [review]
patch

a=mconnor on behalf of 1.9 drivers
Attachment #315067 - Flags: approval1.9+
(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
Whiteboard: [needs unit test]
Attached patch wip (obsolete) — Splinter Review
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...
Was the patch checked in?
(In reply to comment #8)
> Was the patch checked in?

no

Attached patch patch (obsolete) — Splinter Review
- 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.
Attachment #315067 - Attachment is obsolete: true
Attachment #318817 - Attachment is obsolete: true
Attachment #319375 - Flags: review?(dietrich)
Blocks: 432214
Whiteboard: [needs unit test]
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.
Attachment #319375 - Flags: review?(dietrich) → review+
Comment on attachment 319375 [details] [diff] [review]
patch

asking for new approval, more cleanup but also more tests
Attachment #319375 - Flags: approval1.9?
Attachment #315067 - Flags: review+
Attachment #315067 - Flags: approval1.9+
Attached patch for check-inSplinter Review
Attachment #319375 - Attachment is obsolete: true
Attachment #319375 - Flags: approval1.9?
Attachment #319375 - Flags: approval1.9?
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.
Attachment #319375 - Flags: approval1.9?
Comment on attachment 319748 [details] [diff] [review]
for check-in

a+ schrep based on Dietrich's risk assessment over IRC
Attachment #319748 - Flags: approval1.9+
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Priority: -- → P3
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.