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)
Tracking
(seamonkey2.49esr wontfix, seamonkey2.60 wontfix, seamonkey2.53 affected, seamonkey2.57esr fixed)
RESOLVED
FIXED
Future
People
(Reporter: frg, Assigned: frg)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
iannbugzilla
:
review+
iannbugzilla
:
approval-comm-esr60+
|
Details | Diff | Splinter Review |
+++ 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.
Assignee | ||
Comment 1•6 years ago
|
||
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?
Assignee | ||
Comment 2•6 years ago
|
||
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?
Assignee | ||
Comment 5•6 years ago
|
||
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?
Assignee | ||
Comment 6•6 years ago
|
||
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+
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
Assignee | ||
Updated•6 years ago
|
Target Milestone: --- → Future
Assignee | ||
Comment 9•6 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•