Closed Bug 255931 Opened 19 years ago Closed 19 years ago
favicons should validate content type before saving data
Favicons should sniff the content type (using bug 247981 stuff) before saving the icon to bookmarks.html, to make sure we don't save invalid non-image data as an icon.
patch to add sniffing support.
19 years ago
Attachment #156379 - Flags: approval-aviary?
19 years ago
Status: NEW → ASSIGNED
Comment on attachment 156379 [details] [diff] [review] favicon-sniffing-0.patch firstname.lastname@example.org
Attachment #156379 - Flags: approval-aviary? → approval-aviary+
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
It looks like your changes to nsBookmarksService::UpdateBookmarkIcon also fixes the issue of the wrong MIME type being saved in bookmarks.html (eg, text/plain). See: http://bugzilla.mozilla.org/show_bug.cgi?id=174265#c85 Not sure if a seperate bug on the issue was filed elsewhere. Will try to verify this soon.
With todays builds, I can verify the issue I mentioned previously is now fixed. When we receive a BMP icon sent as text/plain, it is now listed in the bookmarks store with the proper type, image/x-icon. However, I noticed a seperate issue while testing this. When I initially bookmark a page, the icon isn't saved. Only after first loading the bookmark is the ICON= attribute added. If we're bookmarking a page that's currently displayed, the icon data should be available for saving in the initial bookmarks update. (and if not, refetech it).
Comment on attachment 156379 [details] [diff] [review] favicon-sniffing-0.patch >Index: src/nsBookmarksService.cpp >+ PRInt32 len = ((aIconDataLen + 2) / 3) * 4; This is not necessary at all. >+ nsString dataUri; >+ dataUri += NS_LITERAL_STRING("data:"); >+ dataUri += NS_ConvertASCIItoUTF16(aMIMEType); >+ dataUri += NS_LITERAL_STRING(";base64,"); >+ dataUri += NS_ConvertASCIItoUTF16(iconDataBase64, len); For the trunk, it'd have been better to use AppendLiteral and AppendASCII in place of '+= NS_LITERAL_STRING' and '+= NS_ConvertASCIItoUTF16'. For the branch, 'dataUri += NS_ConvertASCIItoUTF16(,)' should have been replaced by 'AppendASCIItoUTF16(icondDataBase64)'.
(In reply to comment #6) > For the branch, 'dataUri += NS_ConvertASCIItoUTF16(,)' should have been > replaced by 'AppendASCIItoUTF16(icondDataBase64)'. I meant 'AppendASCIItoUTF16(icondDataBase64, dataUri);' (note PL_base64Encode ? returns a null-terminated string, which is why you don't need to calculate 'len'). The same is true of aMimeType.
sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change
QA Contact: mconnor → bookmarks
You need to log in before you can comment on or make changes to this bug.