Closed Bug 766799 Opened 12 years ago Closed 11 years ago

Redirects visits are not notified anymore through history observers

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox15 --- wontfix
firefox16 --- wontfix
firefox17 --- wontfix
firefox18 --- wontfix
firefox19 --- wontfix

People

(Reporter: darktrojan, Assigned: mak)

References

()

Details

(Keywords: addon-compat, regression)

Attachments

(1 file, 2 obsolete files)

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?)
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.
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.
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 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....
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?
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.
(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.
18.0~a1~hg20120903r104132-0ubuntu1~umd1~precise

RSS still broken http://www.theregister.co.uk/headlines.rss
 (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.
(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.
(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
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.
(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 :-|
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.
Summary: Livemark articles not marked as read until after browser restart → Redirects visits are not notified anymore through history observers
Blocks: 325241
Attached patch patch v1.0 (obsolete) — Splinter Review
This fixes the bug, and I quite like it.  Though, I still have to write a test.
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.
Attached patch patch v1.1 (obsolete) — Splinter Review
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)
Whiteboard: [needs SR]
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
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.
Attached patch patch v1.2Splinter Review
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)
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.
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
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...
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.
Depends on: 821628
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 on attachment 691916 [details] [diff] [review]
patch v1.2

r=mano
Attachment #691916 - Flags: review?(mano) → review+
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
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 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+
Keywords: addon-compat
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.
Flags: in-testsuite+
Whiteboard: [needs SR]
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
https://hg.mozilla.org/mozilla-central/rev/5356d4e7814c
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.