Redirects visits are not notified anymore through history observers

RESOLVED FIXED in mozilla20

Status

()

Toolkit
Places
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: darktrojan, Assigned: mak)

Tracking

({addon-compat, regression})

unspecified
mozilla20
addon-compat, regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox15 wontfix, firefox16 wontfix, firefox17 wontfix, firefox18 wontfix, firefox19 wontfix)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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

5 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

5 years ago
thanks, the figure.fm example is reproducible.
Assignee: nobody → mak77
Duplicate of this bug: 778455

Comment 4

5 years ago
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

5 years ago
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

5 years ago
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

5 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.

Comment 8

5 years ago
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

5 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

5 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

5 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

5 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

5 years ago
18.0~a1~hg20120903r104132-0ubuntu1~umd1~precise

RSS still broken http://www.theregister.co.uk/headlines.rss

Comment 14

5 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

5 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

5 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

5 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

5 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

5 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

5 years ago
Summary: Livemark articles not marked as read until after browser restart → Redirects visits are not notified anymore through history observers
(Assignee)

Updated

5 years ago
status-firefox15: affected → wontfix
status-firefox16: affected → wontfix
status-firefox17: affected → wontfix
status-firefox18: --- → wontfix
status-firefox19: --- → affected
(Assignee)

Updated

5 years ago
Blocks: 325241
(Assignee)

Comment 20

5 years ago
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.
(Assignee)

Comment 21

5 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.
status-firefox19: affected → wontfix
(Assignee)

Comment 22

5 years ago
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.
Attachment #689499 - Attachment is obsolete: true
Attachment #689720 - Flags: review?(mano)
(Assignee)

Updated

5 years ago
Whiteboard: [needs SR]
(Assignee)

Comment 23

5 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

5 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

5 years ago
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.
Attachment #689720 - Attachment is obsolete: true
Attachment #689720 - Flags: review?(mano)
(Assignee)

Comment 26

5 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

5 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

5 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

5 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)

Updated

5 years ago
Depends on: 821628
(Assignee)

Comment 30

5 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 on attachment 691916 [details] [diff] [review]
patch v1.2

r=mano
Attachment #691916 - Flags: review?(mano) → review+

Comment 32

5 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

5 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 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
(Assignee)

Comment 35

5 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

5 years ago
Flags: in-testsuite+
Whiteboard: [needs SR]
(Assignee)

Comment 36

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5356d4e7814c
Target Milestone: --- → mozilla20

Comment 37

5 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
https://hg.mozilla.org/mozilla-central/rev/5356d4e7814c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.