Closed Bug 1489307 Opened 6 years ago Closed 6 years ago

Port Bug 1255270 "Favicon request doesn't timeout, or close when related window is closed" to SeaMonkey

Categories

(SeaMonkey :: General, defect)

SeaMonkey 2.42 Branch
defect
Not set
normal

Tracking

(seamonkey2.49esr wontfix, seamonkey2.60 wontfix, seamonkey2.53 affected, seamonkey2.57esr fixed)

RESOLVED FIXED
Future
Tracking Status
seamonkey2.49esr --- wontfix
seamonkey2.60 --- wontfix
seamonkey2.53 --- affected
seamonkey2.57esr --- fixed

People

(Reporter: frg, Assigned: frg)

References

Details

Attachments

(1 file, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1255270 +++ User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0 Build ID: 20160303134406 Steps to reproduce: The Firefox does background request to load favicon.ico from the current web page. Unexpectedly, the connection is not terminated when the website window is closed which gives an server owner an ability to send data regularly to the browser and figure out when the transmission fails indicating the browser close. It is also possible to measure pings using ACKs. Theoretically, could it be possible to recognize mobile device geological position from a small list of alternative positions based on ping flow? Remember that the pinging is possible as long as browser is on. Actual results: No timeout on favicon connections. Expected results: Timeout on favicon connections.
Depends on: 1378089
Attached patch 1489307-faviconservice-V2.patch (obsolete) (deleted) — Splinter Review
This is more or less a 1:1 patch. The places favicon parts did already go in with Bug 1378089 so all this is needed is just to call them. I was unable to test the code in loadFavIcon. I do not use a proxy. If someone does please check. I believe the previous code in loadFavIcon was not entirely correct. The favicon loading is/was asynchronous for some time. So bailing out if the icon was in the failed favicon cache probably never happened. I moved this into a callback which should only do the setAttribute when successful. This is just a good guess but it did this in the feedwriter code so should be correct. I removed setting the icon in feedwriter.js. This does usually not work. The icon is only set if it was previously fetched with exactly the url in the region.properties which never happend. I can re-add this but it is imho a waste. See part 2 why. 2.49 would need some extensive rebasing. I am quite sure if we have the same sec problem as Fx with no mobile client so I don't think it would be worth it. If you do not agree please let me know and i will do a 2.49 fix.
Attachment #9007066 - Flags: review?(iann_bugzilla)
Attachment #9007066 - Flags: approval-comm-esr60?
Attached patch 1489307-rss-cleanup.patch (obsolete) (deleted) — Splinter Review
aol and digg are now dead. Feedly uses a new api and an account. This would need new code. This leaves yahoo. I don't have an account there but it should still work. I updated the http links to https too. We can not rename all of them because some names are used in toolkit. Should we add a message to localizers or rename internal ones and/or take this part to a new bug?
Attachment #9007068 - Flags: review?(iann_bugzilla)
Attachment #9007068 - Flags: approval-comm-esr60?
Comment on attachment 9007066 [details] [diff] [review] 1489307-faviconservice-V2.patch Thanks r/a=me
Attachment #9007066 - Flags: review?(iann_bugzilla)
Attachment #9007066 - Flags: review+
Attachment #9007066 - Flags: approval-comm-esr60?
Attachment #9007066 - Flags: approval-comm-esr60+
Comment on attachment 9007068 [details] [diff] [review] 1489307-rss-cleanup.patch I would have this as a separate bug. As far as I am aware you still have: browser.contentHandlers.types.1.title=feedly browser.contentHandlers.types.1.uri=https://feedly.com/i/subscription/feed/%s browser.contentHandlers.types.2.title=Inoreader browser.contentHandlers.types.2.uri=https://www.inoreader.com/?add_feed=%s browser.contentHandlers.types.3.title=Netvibes browser.contentHandlers.types.3.uri=https://www.netvibes.com/subscribe.php?url=%s as well as Yahoo. There is also Newsblur, The Old Reader, G2 Reader and Feeder but not sure if they work the same way...
Attachment #9007068 - Flags: review?(iann_bugzilla)
Attachment #9007068 - Flags: approval-comm-esr60?
The patch unfortunately caused a regression. The favicon was only loaded on the first url of a page. Navigating the page cleared it. The callback was never called and loadFavIcon was actually not proxy only as I first thought. I still think this might need to be changed but setting the attribute this way worked before so save refactoring this for a later time. Only one change which removes the added callback in loadFavIcon compared to the first patch.
Attachment #9007066 - Attachment is obsolete: true
Attachment #9007454 - Flags: review?(iann_bugzilla)
Attachment #9007454 - Flags: approval-comm-esr60?
Comment on attachment 9007068 [details] [diff] [review] 1489307-rss-cleanup.patch Will be taken to a new Bug
Attachment #9007068 - Attachment is obsolete: true
Comment on attachment 9007454 [details] [diff] [review] 1489307-faviconservice-V2a.patch r/a=me
Attachment #9007454 - Flags: review?(iann_bugzilla)
Attachment #9007454 - Flags: review+
Attachment #9007454 - Flags: approval-comm-esr60?
Attachment #9007454 - Flags: approval-comm-esr60+
Blocks: 1430240
Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/313ef1e61609 Port Bug 1255270 [Favicon request doesn't timeout, or close when related window is closed] to SeaMonkey. r=IanN
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: