1182046 - RSS history not marked at first load and at auto-refresh

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[laaczy]

Attachment: RSS_first_time.jpg

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150706172413

Steps to reproduce:

Started FF.
Clicked on a live bookmark.


Actual results:

Bookmark loaded, but the previously opened entries are not marked as already visited.
After closing and reopening the live bookmark the historical entries are marked correctly.


Expected results:

Historical entries should be marked at first opening.

Comment 1

[laaczy]

Attachment: RSS_second_time.jpg

Comment 2

[:Gijs (he/him)]

I can reproduce this.

Reporter, do you know if this issue existed with earlier versions of Firefox (ie did this "break" in Firefox 40 or 39 or something?)

Marco, does this ring any bells?

Comment 3

[laaczy]

Tested with 39b7, but there is no error. Seems to be introduced with 40b1.

Comment 4

[Marco Bonardo [:mak]]

I can reproduce it, I don't know what's the cause, sounds like a regression.

Comment 5

[Alice0775 White]

1st regression :  Livemarks folder is empty
Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=1a0302982302&tochange=6126a2cc97c6

Regressed by: 6126a2cc97c6	Marco Bonardo — Bug 1094900 - Livemarks service should use the new Bookmarks.jsm. r=ttaubert

2nd regression : Historical entries does not marked at first opening the popup
Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=a32292906940&tochange=7a5be08b02e0

Regressed by: 	d8590ff0898f	Marco Bonardo — Bug 1159346 - Existing livemarks are broken. r=ttaubert


So, Bug 1159346 did not fix completely.

Comment 6

[Marco Bonardo [:mak]]

thanks, that helps. sounds like we might have a race condition somewhere due to async behavior that replaced sync behavior.

Comment 7

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
This regressed in 40, would be nice if we could fix it in 40.

Comment 8

[Marco Bonardo [:mak]]

40 is behind the corner, I doubt I can find any time to work on this soon enough.

Comment 9

[jen strickland :jenstrickland]

Tracked for 40, 41, and 42, since this sync behavior could confuse and irritate users, so should be fixed.

Comment 10

[Alice0775 White]

[Tracking Requested - why for this release]:

Comment 11

[Ton [:Tonnes]]

Note that this behaviour doesn’t only occur once after startup, but returns randomly afterwards.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Marco, could you help with this bug? Thanks

Comment 14

[Norbert]

I get this problem too.

The "random" recurrence usually happening when the RSS is auto-refreshed. I have 3 RSS feed next to each other in bookmark toolbar, so I can switch between them by moving the mouse, and it clears up when I change to an another RSS and back.

Very annoying tbh.

Comment 15

[Marco Bonardo [:mak]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #13)
> Marco, could you help with this bug? Thanks

how critical is this? I have a lot of regressions on my plate to look into, I can't handle all of them.
While I hate this bug (that is hitting myself since I use RSS everyday), it looks like it may have to wait for more critical work first...

Comment 16

[atlanto]

diff --git a/toolkit/components/places/nsLivemarkService.js b/toolkit/components/places/nsLivemarkService.js
--- a/toolkit/components/places/nsLivemarkService.js
+++ b/toolkit/components/places/nsLivemarkService.js
@@ -647,18 +647,18 @@ Livemark.prototype = {
       }
     }
 
     for (let [ container, observer ] of this._resultObservers) {
       if (this._nodes.has(container)) {
         let nodes = this._nodes.get(container);
         for (let node of nodes) {
           // Workaround for bug 449811.
-          localObserver = observer;
-          localNode = node;
+          let localObserver = observer;
+          let localNode = node;
           if (!aURI || node.uri == aURI.spec) {
             Services.tm.mainThread.dispatch(() => {
               localObserver.nodeHistoryDetailsChanged(localNode, 0, aVisitedStatus);
             }, Ci.nsIThread.DISPATCH_NORMAL);
           }
         }
       }
     }

Comment 17

[Marco Bonardo [:mak]]

Good catch, does that solve the problem for you? would you mind making a proper diff?
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 18

[Marco Bonardo [:mak]]

Does anyone have reliable steps to reproduce the bug? I could reproduce in my everyday profile but randomly. I have difficulties reproducing it in a clean profile.
Having reliable str would help checking if the patch in comment 16 (that we will take regardless) actually fixes this issue.

Comment 19

[Marco Bonardo [:mak]]

Attachment: fix undeclared variables

so, I'd say to land this, leave-open the bug and see if the issue is gone in Nightly. We can mark as fixed at a later time.

Comment 20

[Alice0775 White]

my str
1. Start Nightly with disabled e10s
2. Enabled Bookmarks Toolbar
3. Open http://fxfeeds.mozilla.com/en-US/firefox/headlines.xml
4. Select "Live Bookmarks" and click [Subscribe Now], and then click [Subscribe]

5. Open "BBC News home" livebookmarks folder, and then click some entry items


6. Restart Nightly
7. Open the livebookmarks folder
   --- observe all entries are marked as unvisited --- Bug
8. Open the livebookmarks folder again
   --- observe the entries are marked as visited 


Actual Results:
 At Step7, all entries are marked as unvisited

Expected Results:
 At Step7, the entries should be marked as visited

Comment 21

[Jan Henning [:JanH]]

I can easily reproduce this on a fresh profile, the only thing I have to do is disable e10s, because with e10s enabled, the interface for viewing RSS feeds and adding a live bookmarks is broken (is there already a bug for that?).

Basically it's:
- Add a live bookmark to your bookmarks menu, it shouldn't matter which one, but I've tested with http://www.kvv.de/ticker_rss.xml
- Visit a few entries from the live bookmark, so they show up as visited.
- Close Firefox and then restart it.
- Open the bookmarks menu and move the cursor to the live bookmark - the sub-menu will open with all entries showing as unread.
- Move the cursor away so the live bookmark sub-menu closes, then move it back again. This time around, visited entries will show up correctly.

Comment 22

[Loic]

(In reply to Marco Bonardo [::mak] from comment #18)
> Does anyone have reliable steps to reproduce the bug? I could reproduce in
> my everyday profile but randomly. I have difficulties reproducing it in a
> clean profile.

It's 100% reproducible with RSS feeds displayed in the bookmarks sidebar (Ctrl+B). When you click on a RSS entry to read it, the icon of the feed doesn't change to show the feed has been read (it stays orange).

Comment 24

[Marco Bonardo [:mak]]

Thanks, the patch seems to be working, for some reason I couldn't reproduce with my previous build, but I could on a clean one.

Comment 25

[Loic]

Attachment: rssfeed.jpg

The weird thing I observed with this bug is only the last feed is marked as read when you click on it, all the otehrs stay unread.

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5e9ff895b721

Comment 27

[Marco Bonardo [:mak]]

Comment on attachment 8657436 [details] [diff] [review]
fix undeclared variables

Approval Request Comment
[Feature/regressing bug #]: async bookmarks
[User impact if declined]: feed entries are not properly marked as visited
[Describe test coverage new/current, TreeHerder]: Nightly
[Risks and why]: no risk, trivial syntax fix
[String/UUID change made/needed]: none

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e9ff895b721

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Alice0775, Laaczy: could you please verify that the fix works as expected on Nightly 09-09 or later? Thanks.

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8657436 [details] [diff] [review]
fix undeclared variables

Fix seems simple enough, let's uplift to Aurora42 and Beta41.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f5dd5cd1f96f

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/eaeab9ffd16c

Comment 33

[Alice0775 White]

(In reply to Ritu Kothari (:ritu) from comment #29)
> Alice0775, Laaczy: could you please verify that the fix works as expected on
> Nightly 09-09 or later? Thanks.

I can verify that the problem is fixed on latest Nightly43.0a1(2015-09-09)

https://hg.mozilla.org/mozilla-central/rev/01ae99b53561a2c3b40533d8c1c92bd3efc42d00
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0 ID:20150909030223

Comment 35

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Alice0775 White from comment #33)
> (In reply to Ritu Kothari (:ritu) from comment #29)
> > Alice0775, Laaczy: could you please verify that the fix works as expected on
> > Nightly 09-09 or later? Thanks.
> 
> I can verify that the problem is fixed on latest Nightly43.0a1(2015-09-09)
> 
> https://hg.mozilla.org/mozilla-central/rev/
> 01ae99b53561a2c3b40533d8c1c92bd3efc42d00
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0
> ID:20150909030223

Thanks Alice0775! Verified.

Comment 36

[laaczy]

I'm on beta channel. Will be the fix part of the next beta on Friday? (41b9)
I can only test it afterward.

Comment 37

[Andrei Vaida [:avaida]]

(In reply to laaczy from comment #36)
> I'm on beta channel. Will be the fix part of the next beta on Friday? (41b9)
> I can only test it afterward.

laaczy, verifying this fix on Firefox 41.0 Beta 9 would be greatly appreciated!

You'll be able to download the 41.0b9 build tomorrow, using the following link:
https://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/41.0b9-candidates/build1/

Comment 38

[laaczy]

The error is fixed on 41b9.

Comment 39

[:Gijs (he/him)]

(In reply to laaczy from comment #38)
> The error is fixed on 41b9.

Thank you!

Comment 40

[Ton [:Tonnes]]

It might be a coincidence, but occasionally the feed menu appears empty on first click and needs a second click to show the items. I don’t recall seeing this in previous versions before this bug was filed.
Could it be related / anyone else seeing this, and would it need a new bug?

Comment 41

[flyer185]

I see it that way when I haven't used it for some time, but if I wait 5-10 seconds it then populates normally. It's almost as if FFox decides it has been long enough that it wants to start from scratch.

Comment 42

[Marco Bonardo [:mak]]

Sometimes the contents must be fetched from the network, the time it takes depends on the network...

Comment 43

[Ton [:Tonnes]]

(In reply to flyer185 from comment #41 and Marco Bonardo [::mak] from comment #42)

I’m afraid this is not about some delay in fetching data, but a pure display issue. The RSS feed doesn’t open, but only shows the right side border of the list (a vertical black line), and it stays that way - waiting for it to load doesn’t help reloading the items, even for a minute.
I just reproduced it by quickly clicking the feed name (on the Bookmarks Toolbar) multiple times in a row. It happens about 1 out of 10 times, and the list immediately loads as it should at the next click right after.

Also tried to add a website feed in current Aurora in order to check in Safe Mode but for some reason adding the feed didn’t work, which worries me as well.

Comment 44

[Marco Bonardo [:mak]]

ok, doesn't sound like there may be a relation with this bug, it's worth its own bug. May be something  at the widget/gfx level, but could be very hard to figure out the cause.

Comment 45

[Ton [:Tonnes]]

(In reply to Marco Bonardo [::mak] from comment #44)
> ok, doesn't sound like there may be a relation with this bug, it's worth its
> own bug. May be something  at the widget/gfx level, but could be very hard
> to figure out the cause.

IMO, the relationship with empty feed lists to this bug is
1) I understand there were some doubts above about fixing this bug the proper way, so it might have caused the display issue as a regression, and
2) the issue I notice now is there ever since this bug occurred and got fixed,

… so I thought it was good to mention it here first. Would it need a new bug anyway?

The other issue with a failing Subcribing button / menu entry is unrelated allright, got introduced here with Aurora 43 apparently, and seems to ahve been filed as bug 1213159.

Comment 46

[:Gijs (he/him)]

(In reply to Ton from comment #45)
> The other issue with a failing Subcribing button / menu entry is unrelated
> allright, got introduced here with Aurora 43 apparently, and seems to ahve
> been filed as bug 1213159.

As I noted there, that's an e10s bug. What you're seeing is likely different unless you have e10s turned on.

Comment 47

[Marco Bonardo [:mak]]

(In reply to Ton from comment #45)
> … so I thought it was good to mention it here first. Would it need a new bug
> anyway?

Once a bug is resolved, it's either reopened (if the bug still exists) or done. a New bug is needed regardless a relation, there are fields to mark relations between bugs.