Closed
Bug 766799
Opened 12 years ago
Closed 12 years ago
Redirects visits are not notified anymore through history observers
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: darktrojan, Assigned: mak)
References
()
Details
(Keywords: addon-compat, regression)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
asaf
:
review+
mossop
:
superreview+
|
Details | Diff | Splinter Review |
To quote reddit:
"A new update has the orange mark next to it. I click on it and go to the page, but when I'm in the same session the mark is not switched to grey/read. Once I close and restart Firefox it is then marked read.
Any idea how to fix this? Works fine in FF13. Doesn't work even with all extensions disabled."
I see this too, suspect it's something to do with how we handle redirects? (bug 737841?)
Comment 1•12 years ago
|
||
I have noticed this also, I think since 14.0a2 (only using Aurora), but not in every RSS feed: some remain orange after read (e.g. "figure.fm: Items" at http://www.figure.fm/, sometimes NSFW) and some turn grey as expected (e.g. "Paul Krugman" at http://krugman.blogs.nytimes.com/).
Alas, I don't need a browser restart: Right Click -> Reload Live Bookmark updates this state in the affected feeds. Hopefully this is the issue at hand.
Assignee | ||
Comment 2•12 years ago
|
||
thanks, the figure.fm example is reproducible.
Assignee: nobody → mak77
RSS still broken TESTING: firefox-trunk-globalmenu (17.0~a1~hg20120731r100965-0ubuntu1~umd1~precise
RSS - Live Bookmarks not showing as read on most feeds in FF14-17a (13 OK)
Example, click on an item on "The Register" Feed Location: http://www.theregister.co.uk headlines.rss.
Orange icon in the menu should change from orange to grey indicating that the item has been read.
But the icon in menu actually stays orange.
The same happens with Ars Technica and most other sites but not with the BBC.
Workaround:
Right clicking "reload Live Bookmarks" updates the icons correctly.
Note:
Name: The Register
Feed Location: http://www.theregister.co.uk/headlines.rss
Site: Location: http://www.theregister.co.uk/
Name: Ars Technica
Feed Location: http://feeds.arstechnica.com/arstechnica/everything?format=xml
Site: Location: http://arstechnica.com/
Name: BBC News - Home
Feed Location: http://feeds.bbci.co.uk/news/rss.xml?edition=int
Site: Location: http://www.bbc.co.uk/news/#sa-ns_mchannel=rss&ns_source=PublicRSS20-sa
---
Firefox-Trunk
Version 17.0a1
User Agent Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:17.0) Gecko/17.0 Firefox/17.0
Extensions Name Version Enabled ID
Adblock Plus 2.1.2 true {d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d}
Global Menu Bar integration 3.4pre true globalmenu@ubuntu.com
HackTheWeb 1.3.2 true hacktheweb@instantfox.com
Ubuntu Firefox Modifications 2.1.1 true ubufox@ubuntu.com
Bug Name change needed:
Is: Bug 766799 - Livemark articles not marked as read until after browser restart.
Should be: Bug 766799 - Livemark articles not marked as read until after Reload Live Bookmark.
This is not a valid workaround either:
http://www.howtogeek.com/howto/internet/firefox/change-update-interval-of-live-bookmarks-in-firefox/
Assignee | ||
Comment 7•12 years ago
|
||
the problem is likely due to some internal redirect in the page, so there's likely nothing you can do client-side to workaround it.
Same issue with:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20100101 Firefox/14.0.1 ID:20120713134347
Not Working:
http://rss.slashdot.org/Slashdot/slashdot
http://www.h-online.com/grand-atom.xml
http://feeds.slashgear.com/slashgear?format=xml
http://feeds.computerworld.com/Computerworld/TopNews
http://feeds.arstechnica.com/arstechnica/everything?format=xml
Working OK#
http://penny-arcade.com/feed
http://www.theverge.com/rss/index.xml
http://www.groklaw.net/backend/GrokLaw.rdf
http://newsrss.bbc.co.uk/rss/newsonline_world_edition/front_page/rss.xml
Comment 9•12 years ago
|
||
Same but different issue with:
http://planet.mozilla.org/projects/rss20.xml
http://planet.mozilla.org/atom.xml
Mostly there is no way to get them marked as read.
Some work but most not at all....
Comment 10•12 years ago
|
||
Marco could I ask if you are making any progress on fixing this issue?
I am at present using using 13.0.1 as more up to date versions break rss functionality.
If you don’t see a way to fix this should I be looking into using Firefox ESR?
Comment 11•12 years ago
|
||
FWIW, the LiveClick extension author has a beta version of his extension at http://projects.protej.com/index.php/liveclick/comments/open_beta_3_liveclick_0500/ that handles rss properly and can be used to work around this issue in current fx versions.
Comment 12•12 years ago
|
||
(In reply to custom.firefox.lady from comment #11)
> FWIW, the LiveClick extension author has a beta version of his extension at
> http://projects.protej.com/index.php/liveclick/comments/
> open_beta_3_liveclick_0500/ that handles rss properly and can be used to
> work around this issue in current fx versions.
Liveclick has stopped working on the latest Nightly.
Firefox 17 renamed one of the functions that LiveClick uses when checking for new items. The next LiveClick update will address this problem.
Comment 13•12 years ago
|
||
18.0~a1~hg20120903r104132-0ubuntu1~umd1~precise
RSS still broken http://www.theregister.co.uk/headlines.rss
Comment 14•12 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #7)
> the problem is likely due to some internal redirect in the page, so there's
> likely nothing you can do client-side to workaround it.
As 90% of my Livemark articles not marked as read until I manually select "Reload Live Bookmarks", I suggest that the fix is to automatically "Reload Live Bookmarks" after each Livemark article selection.
Note that by actively clicking on Livemark articles, I have indicated that I am prepared to pay the extra overhead.
There would be no extra hit for ppl NOT using Livemarks.
Assignee | ||
Comment 15•12 years ago
|
||
(In reply to silverwav from comment #14)
> As 90% of my Livemark articles not marked as read until I manually select
> "Reload Live Bookmarks", I suggest that the fix is to automatically "Reload
> Live Bookmarks" after each Livemark article selection.
Which OS? note Mac is "bogus" and will keep being, regardless this fix.
Comment 16•12 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #15)
> (In reply to silverwav from comment #14)
> > As 90% of my Livemark articles not marked as read until I manually select
> > "Reload Live Bookmarks", I suggest that the fix is to automatically "Reload
> > Live Bookmarks" after each Livemark article selection.
>
> Which OS? note Mac is "bogus" and will keep being, regardless this fix.
Linux - Ubuntu 64bit 12.04
Firefox-Trunk
Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:18.0) Gecko/18.0 Firefox/18.0
18.0~a1~hg20120903r104132-0ubuntu1~umd1~precise
Win7 - 64bit Professional
Firefox
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0
Assignee | ||
Comment 17•12 years ago
|
||
So, this is regression in Firefox 14, due to the fact redirects are now considered hidden pages, and we don't notify visits for hidden pages. Likely we may reconsider notifying since hidden has a slightly different meaning from the original ones.
Though I have to double verify it won't bring unexpected perf regressions.
Blocks: 737841
status-firefox15:
--- → affected
status-firefox16:
--- → affected
status-firefox17:
--- → affected
Keywords: regression
Comment 18•12 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #17)
> So, this is regression in Firefox 14, due to the fact redirects are now
> considered hidden pages, and we don't notify visits for hidden pages. Likely
> we may reconsider notifying since hidden has a slightly different meaning
> from the original ones.
Thank God for that. I am still running Firefox 13 as my daily driver, (as its the last one that worked.
Now I’m on Linux, so fairly safe, but still...
I would like to be able to upgrade from Firefox 13 sometime this year :-|
Comment 19•12 years ago
|
||
I understand this is low priority, but for those who rely on Firefox to read their RSS feeds, this is a major usability problem.
If there is any chance to upgrade this ticket in priority, that would be much appreciated.
Assignee | ||
Updated•12 years ago
|
Summary: Livemark articles not marked as read until after browser restart → Redirects visits are not notified anymore through history observers
Assignee | ||
Updated•12 years ago
|
status-firefox18:
--- → wontfix
status-firefox19:
--- → affected
Assignee | ||
Comment 20•12 years ago
|
||
This fixes the bug, and I quite like it. Though, I still have to write a test.
Assignee | ||
Comment 21•12 years ago
|
||
The fix ends up being a bit more complicated than initially thought, so it's really unlikely to backport it. But it's almost ready, 1 test failure to go.
Assignee | ||
Comment 22•12 years ago
|
||
In the end, apart notifying hidden visits, this ended up fixing other 3 bugs thanks to the tests:
- a warning due to a possibly overflowing constant
- a sort by frecency error
- broken liveupdate of searches without includeHidden
it's currently running on try, should post results later.
Attachment #689499 -
Attachment is obsolete: true
Attachment #689720 -
Flags: review?(mano)
Assignee | ||
Updated•12 years ago
|
Whiteboard: [needs SR]
Assignee | ||
Comment 23•12 years ago
|
||
looks like the try server forgets to send back data to bugzilla... btw for some reason xpcshell is failing only on Mac:
TEST-UNEXPECTED-FAIL | /builds/slave/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/unit/test_history_observer.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/slave/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/unit/test_history_observer.js | true == false - See following stack:
and looks like this test needs a fix
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/places/tests/browser/browser_redirect.js | The redirect source should not be notified
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/places/tests/browser/browser_redirect.js | The target page should not be hidden - Got -1, expected 0
Assignee | ||
Comment 24•12 years ago
|
||
the bc failure was indeed the test in need of a fix, it was not expecting the redirect notification. I've not yet identified the cause of the other Mac-only failure.
Assignee | ||
Comment 25•12 years ago
|
||
unbitrot against recent test changes, this is now using the async visits addition.
Running on try server, will ask review once green.
Attachment #689720 -
Attachment is obsolete: true
Attachment #689720 -
Flags: review?(mano)
Assignee | ||
Comment 26•12 years ago
|
||
still failing on Mac, but not always, looks like it's intermittent but happens more often on Mac. Likely the threads scheduler is showing me a bug.
Comment 27•12 years ago
|
||
Try run for 8808da1f7f4f is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=8808da1f7f4f
Results (out of 77 total builds):
success: 61
warnings: 16
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mak77@bonardo.net-8808da1f7f4f
Assignee | ||
Comment 28•12 years ago
|
||
funny, gdb says the value is false, but when it reaches the js side, it becomes true. Looks like there is something weird in the xpcstub, the onVisit method depletes all of the integers registers, thus the bool should be in args (stack), but I can't find it there...
Assignee | ||
Comment 29•12 years ago
|
||
confirmed that this is definitely a bug in the xpcstub, basically we have to do the same fix as in bug 690668 to the other x64 stubs.
Assignee | ||
Comment 30•12 years ago
|
||
Comment on attachment 691916 [details] [diff] [review]
patch v1.2
OK the patch is correct, XPCOM is not, but I'm handling that apart in bug 821628
Attachment #691916 -
Flags: review?(mano)
Comment 31•12 years ago
|
||
Comment on attachment 691916 [details] [diff] [review]
patch v1.2
r=mano
Attachment #691916 -
Flags: review?(mano) → review+
Comment 32•12 years ago
|
||
Try run for 98e50713601e is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=98e50713601e
Results (out of 130 total builds):
exception: 1
success: 114
warnings: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mak77@bonardo.net-98e50713601e
Assignee | ||
Comment 33•12 years ago
|
||
Comment on attachment 691916 [details] [diff] [review]
patch v1.2
The out param I'm changing is expected to be unused, we wrongly exposed it in the far past when actually we didn't need to (in this patch I properly expose the out param only internally). It's of no use for anyone but our internal code, that's why I feel safe changing it.
Attachment #691916 -
Flags: superreview?(dtownsend+bugmail)
Comment 34•12 years ago
|
||
Comment on attachment 691916 [details] [diff] [review]
patch v1.2
Review of attachment 691916 [details] [diff] [review]:
-----------------------------------------------------------------
Let's flag this for jorge to comment on in an add-ons blog post just in case and keep an eye out for any large breakage but I think this should be fine.
Attachment #691916 -
Flags: superreview?(dtownsend+bugmail) → superreview+
Updated•12 years ago
|
Keywords: addon-compat
Assignee | ||
Comment 35•12 years ago
|
||
based on the usefulness of the previous outparam (as the comment stated the only use is internal) I think this won't break anyone. I could find a couple add-ons defining all onVisit params, included "added" but then they don't use it, cause it's indeed pointless.
The most interesting thing to notify to add-ons is that now we notify all the visits added to the database, and it's up to them to filter hidden pages (redirect sources and framed links) if they are not interested in them.
Assignee | ||
Updated•12 years ago
|
Flags: in-testsuite+
Whiteboard: [needs SR]
Assignee | ||
Comment 36•12 years ago
|
||
Target Milestone: --- → mozilla20
Comment 37•12 years ago
|
||
Try run for 76137fdd735f is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=76137fdd735f
Results (out of 189 total builds):
success: 148
warnings: 39
failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mak77@bonardo.net-76137fdd735f
Comment 38•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•