Last Comment Bug 766799 - Redirects visits are not notified anymore through history observers
: Redirects visits are not notified anymore through history observers
Status: RESOLVED FIXED
: addon-compat, regression
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: unspecified
: All All
: -- normal with 2 votes (vote)
: mozilla20
Assigned To: Marco Bonardo [::mak] (Away 6-20 Aug)
:
Mentors:
http://www.reddit.com/r/firefox/comme...
: 778455 (view as bug list)
Depends on: 821628
Blocks: 325241 737841
  Show dependency treegraph
 
Reported: 2012-06-20 17:25 PDT by Geoff Lankow (:darktrojan)
Modified: 2012-12-23 08:37 PST (History)
13 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
wontfix
wontfix
wontfix


Attachments
patch v1.0 (11.99 KB, patch)
2012-12-06 17:48 PST, Marco Bonardo [::mak] (Away 6-20 Aug)
no flags Details | Diff | Splinter Review
patch v1.1 (42.73 KB, patch)
2012-12-07 07:04 PST, Marco Bonardo [::mak] (Away 6-20 Aug)
no flags Details | Diff | Splinter Review
patch v1.2 (44.80 KB, patch)
2012-12-13 11:26 PST, Marco Bonardo [::mak] (Away 6-20 Aug)
asaf: review+
dtownsend: superreview+
Details | Diff | Splinter Review

Description Geoff Lankow (:darktrojan) 2012-06-20 17:25:53 PDT
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 Mauro Sanabria 2012-06-25 08:03:10 PDT
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.
Comment 2 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-06-25 08:58:06 PDT
thanks, the figure.fm example is reproducible.
Comment 3 Virgil Dicu [:virgil] [QA] 2012-07-31 09:14:58 PDT
*** Bug 778455 has been marked as a duplicate of this bug. ***
Comment 4 silverwav 2012-08-04 09:08:12 PDT
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
Comment 5 silverwav 2012-08-04 09:11:30 PDT
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.
Comment 6 silverwav 2012-08-04 09:21:29 PDT
This is not a valid workaround either:

http://www.howtogeek.com/howto/internet/firefox/change-update-interval-of-live-bookmarks-in-firefox/
Comment 7 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-08-22 05:52:51 PDT
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.
Comment 9 Antti Tervasmäki 2012-08-27 11:40:50 PDT
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 silverwav 2012-08-28 23:07:49 PDT
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 custom.firefox.lady 2012-08-30 18:26:04 PDT
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 silverwav 2012-08-31 11:22:01 PDT
(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 silverwav 2012-09-03 11:11:25 PDT
18.0~a1~hg20120903r104132-0ubuntu1~umd1~precise

RSS still broken http://www.theregister.co.uk/headlines.rss
Comment 14 silverwav 2012-09-03 17:28:32 PDT
 (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.
Comment 15 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-09-04 05:18:28 PDT
(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 silverwav 2012-09-04 09:40:32 PDT
(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
Comment 17 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-09-20 17:10:12 PDT
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.
Comment 18 silverwav 2012-09-21 05:11:24 PDT
(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 Max Baranovskii 2012-09-24 12:51:52 PDT
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.
Comment 20 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-06 17:48:58 PST
Created attachment 689499 [details] [diff] [review]
patch v1.0

This fixes the bug, and I quite like it.  Though, I still have to write a test.
Comment 21 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-07 06:46:20 PST
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.
Comment 22 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-07 07:04:21 PST
Created attachment 689720 [details] [diff] [review]
patch v1.1

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.
Comment 23 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-08 02:33:49 PST
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
Comment 24 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-08 04:50:52 PST
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.
Comment 25 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-13 11:26:28 PST
Created attachment 691916 [details] [diff] [review]
patch v1.2

unbitrot against recent test changes, this is now using the async visits addition.

Running on try server, will ask review once green.
Comment 26 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-13 13:09:36 PST
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 Mozilla RelEng Bot 2012-12-13 14:30:37 PST
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
Comment 28 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-13 18:38:27 PST
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...
Comment 29 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-13 19:14:36 PST
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.
Comment 30 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-14 01:35:01 PST
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
Comment 31 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-12-17 02:35:14 PST
Comment on attachment 691916 [details] [diff] [review]
patch v1.2

r=mano
Comment 32 Mozilla RelEng Bot 2012-12-18 07:00:49 PST
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 33 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-20 14:05:22 PST
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.
Comment 34 Dave Townsend [:mossop] 2012-12-20 14:19:25 PST
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.
Comment 35 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-20 14:32:47 PST
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.
Comment 36 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-12-20 14:51:07 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/5356d4e7814c
Comment 37 Mozilla RelEng Bot 2012-12-21 15:19:24 PST
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 :Ms2ger (⌚ UTC+1/+2) 2012-12-22 06:55:05 PST
https://hg.mozilla.org/mozilla-central/rev/5356d4e7814c

Note You need to log in before you can comment on or make changes to this bug.