Closed Bug 1317632 Opened 7 years ago Closed 6 years ago

Reading list folder count does not match items in folder

Categories

(Firefox for Android Graveyard :: General, defect, P1)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect

Tracking

(fennec51+, firefox53 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
fennec 51+ ---
firefox53 --- verified

People

(Reporter: sebastian, Assigned: cnevinchen)

References

Details

Attachments

(1 file)

Bug 1317632 - If the bookmark to add is a reader mode page, defer db action till it cache completely and save the same url in bookmarks and uriannotations table to avoid URL chagne between two http requests.
59 bytes, text/x-review-board-request
sebastian
: review+
Details 
In the bookmarks panel I see "1 item" in my reading list. But when opening the folder then it's actually empty.
This seems to happen whenever I add a medium post. The counter goes up but the page does not show up in the reading list.
tracking-fennec: --- → ?
tracking-fennec: ? → 51+
Assignee: nobody → cnevinchen
Hi Jim
This issue only happens in Medium posts.

For example. This post
https://slackhq.com/wired-founder-kevin-kelly-on-letting-go-of-ai-anxiety-bcd94e50dedc#.6b7jjte9z

Below is the message I got in 
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/Tabs.java#478

After "type":"Content:LocationChange", the end of the url is repeated (%23.k5r9kcul4#.k5r9kcul).

Although it can still display the url correctly, it makes the bookmark display incorrectly ( the url is changed so it's can't be found in the bookmark db. To be precise, it's bookmark_with_annotation view in the db)

Could you please advise if what may go wrong? thanks!

12-08 09:54:02.408 21699-21770/org.mozilla.fennec_nechen D/happy: handleMessage: {"type":"Content:StateChange","tabID":1,"uri":"about:reader?url=https%3A%2F%2Fwritingcooperative.com%2Fyou-care-too-much-about-your-manuscript-b2151faf9d17%23.k5r9kcul4","state":983041,"restoring":false,"success":true}
12-08 09:54:02.472 21699-21770/org.mozilla.fennec_nechen D/happy: handleMessage: {"type":"Content:LocationChange","tabID":1,"uri":"about:reader?url=https%3A%2F%2Fwritingcooperative.com%2Fyou-care-too-much-about-your-manuscript-b2151faf9d17%23.k5r9kcul4","userRequested":"","baseDomain":"writingcooperative.com","contentType":"text\/html","sameDocument":false,"historyIndex":13,"historySize":14,"canGoBack":true,"canGoForward":false}
12-08 09:54:02.489 21699-21770/org.mozilla.fennec_nechen D/happy: handleMessage: {"type":"Content:SecurityChange","tabID":1,"identity":{"origin":"null","mode":{"identity":"unknown","mixed_display":"unknown","mixed_active":"unknown","tracking":"unknown"},"secure":false}}
12-08 09:54:02.622 21699-21770/org.mozilla.fennec_nechen D/happy: handleMessage: {"type":"DOMTitleChanged","tabID":1,"title":""}
12-08 09:54:02.816 21699-21770/org.mozilla.fennec_nechen D/happy: handleMessage: {"type":"Content:PageShow","tabID":1,"userRequested":"","fromCache":false}
12-08 09:54:02.842 21699-21770/org.mozilla.fennec_nechen D/happy: handleMessage: {"type":"Content:StateChange","tabID":1,"uri":"about:reader?url=https%3A%2F%2Fwritingcooperative.com%2Fyou-care-too-much-about-your-manuscript-b2151faf9d17%23.k5r9kcul4","state":786448,"restoring":false,"success":true}
12-08 09:54:03.021 21699-21770/org.mozilla.fennec_nechen D/happy: handleMessage: {"type":"Content:LocationChange","tabID":1,"uri":"about:reader?url=https%3A%2F%2Fwritingcooperative.com%2Fyou-care-too-much-about-your-manuscript-b2151faf9d17%23.k5r9kcul4#.k5r9kcul4","userRequested":"","baseDomain":"writingcooperative.com","contentType":"text\/html","sameDocument":true,"historyIndex":14,"historySize":15,"canGoBack":true,"canGoForward":false}
12-08 09:54:03.188 21699-21770/org.mozilla.fennec_nechen D/happy: handleMessage: {"type":"DOMTitleChanged","tabID":1,"title":"You Care Too Much About Your Manuscript – The Writing Cooperative"}
12-08 09:54:03.433 21699-21770/org.mozilla.fennec_nechen D/happy: handleMessage: {"type":"Link:Favicon","tabID":1,"href":"https:\/\/writingcooperative.com\/favicon.ico","size":0,"mime":""}

After loading the url,
Flags: needinfo?(nchen)
Steps to reproduce :

1. In Android, go to Medium's first page and copy any link of a post to observe the url, it'll be  
    https://writingcooperative.com/Fyou-care-too-much-about-your-manuscript-b2151faf9d17?source=reading_list---------2-3--------

2. Now click the link, it'll be  
   https://writingcooperative.com/Fyou-care-too-much-about-your-manuscript-b2151faf9d17#.k5r9kcul4

3. After you click "reader mode" icon to enable reader mode, in the url will have an additional "#.k5r9kcul4" added like below
   https://writingcooperative.com/Fyou-care-too-much-about-your-manuscript-b2151faf9d17%23.k5r9kcul4#.k5r9kcul4

4. Message I got in https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/Tabs.java#478
   {"type":"Content:StateChange","tabID":1,"uri":"about:reader?url=https%3A%2F%2Fwritingcooperative.com%2Fyou-care-too-much-about-your-manuscript-b2151faf9d17%23.k5r9kcul4","state":983041,"restoring":false,"success":true}

   {"type":"Content:LocationChange","tabID":1,"uri":"about:reader?url=https%3A%2F%2Fwritingcooperative.com%2Fyou-care-too-much-about-your-manuscript-b2151faf9d17%23.k5r9kcul4","userRequested":"","baseDomain":"writingcooperative.com","contentType":"text\/html","sameDocument":false,"historyIndex":13,"historySize":14,"canGoBack":true,"canGoForward":false}

   {"type":"Content:SecurityChange","tabID":1,"identity":{"origin":"null","mode":{"identity":"unknown","mixed_display":"unknown","mixed_active":"unknown","tracking":"unknown"},"secure":false}}

   {"type":"DOMTitleChanged","tabID":1,"title":""}
 
   {"type":"Content:PageShow","tabID":1,"userRequested":"","fromCache":false}

   {"type":"Content:StateChange","tabID":1,"uri":"about:reader?url=https%3A%2F%2Fwritingcooperative.com%2Fyou-care-too-much-about-your-manuscript-b2151faf9d17%23.k5r9kcul4","state":786448,"restoring":false,"success":true}

   {"type":"Content:LocationChange","tabID":1,"uri":"about:reader?url=https%3A%2F%2Fwritingcooperative.com%2Fyou-care-too-much-about-your-manuscript-b2151faf9d17%23.k5r9kcul4#.k5r9kcul4","userRequested":"","baseDomain":"writingcooperative.com","contentType":"text\/html","sameDocument":true,"historyIndex":14,"historySize":15,"canGoBack":true,"canGoForward":false}

   {"type":"DOMTitleChanged","tabID":1,"title":"You Care Too Much About Your Manuscript – The Writing Cooperative"}

   {"type":"Link:Favicon","tabID":1,"href":"https:\/\/writingcooperative.com\/favicon.ico","size":0,"mime":""}


btw, I can't reproduce it on desktop. In desktop, the additional "#.k5r9kcul4" will not be added.
I'm not familiar with reader mode code. Could be an issue with AboutReader.jsm? Does this happen on desktop?
Flags: needinfo?(nchen)
Ahunt has been working on this in the past and might have an idea what's going wrong (I think we fixed something similar in the past?).
Flags: needinfo?(ahunt)
This is a http://medium.com specific problem.

I've checked AboutReader.jsm and ReaderMode.jsm
Turns out it happens in onLocationChange in browser.js
https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#4190

When you click an article on medium, the website will redirect you to the original content URL and add an extra string to the url (e.g. "#.xxxxxxxxx").
After the user clicks "reader" icon to enter reader mode, browser.js's onLocationChange will be triggered twice.
And when the second time onLocationChange() is triggered, the URL will be "oringal_url_ends_with_%23.xxxxxxxxx#.xxxxxxxxx" ==> An extra "#.xxxxxxxxx" is appended.

I'll look for the caller of onLocationChange() and see what went wrong.
I'm now guessing that it's WebNavigationContent.js or RemoteWebProgress.jsm.  I'll try this tomorrow.


Hi Tim
Could you please give me some hint if I'm going to the right direction?
Flags: needinfo?(timdream)
Turns out it's a medium.com's issue.
Search for "medium.com url hashtag" on the web you'll find some complains about duplicated bookmarks caused by the hashtag medium.com append to the URL.
e.g. https://news.ycombinator.com/item?id=11035369
 
If we want to make the URL unique, we need to strike out the hashtag for medium.com.

Hi Sebastian
Do you think I should do some hacking specifically for medium.com?
Flags: needinfo?(s.kaspari)
(In reply to Nevin Chen [:nechen] from comment #6)
> When you click an article on medium, the website will redirect you to the
> original content URL and add an extra string to the url (e.g. "#.xxxxxxxxx").
> After the user clicks "reader" icon to enter reader mode, browser.js's
> onLocationChange will be triggered twice.
> And when the second time onLocationChange() is triggered, the URL will be
> "oringal_url_ends_with_%23.xxxxxxxxx#.xxxxxxxxx" ==> An extra "#.xxxxxxxxx"
> is appended.

Locate the code that trigger this will be helpful. Either the first one (started with escaped %23...) should be stripped out or the second hash should not be added after-the-fact.

Having this problem only happen on Android sounds like a Fennec specific URL normalization issue.

(In reply to Nevin Chen [:nechen] from comment #7)
> Hi Sebastian
> Do you think I should do some hacking specifically for medium.com?

We should not special case anything. We are not that desperate. :)
Flags: needinfo?(timdream)
Flags: needinfo?(s.kaspari)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #8)
> Having this problem only happen on Android sounds like a Fennec specific URL
> normalization issue.

Actually ... this is probably not related since desktop has no "reading list" feature.
We actually have two separate issues going on here:
- 1. Some articles aren't being saved to the reading list correctly
- 2. The item count is based on the number of "reader_view" URL annotations, but the list is populated using the actually cached articles

I.e. whenever we save an offline item
- The item is stored into the offline reader view "cache", the URL is added as a "reader_view" item in UrlAnnotations
- A bookmark is added.

Due to (1), the bookmark URL somehow doesn't match the "cache" URL.

For (2) we'd need to modify getBookmarkCountForFolder() to use the same query that we use in getReadingListBookmarks() - but that only covers up the underlying issue.

We'd also want to figure out why the URLs aren't being correctly processed (and ideally run a fixup migration), which would fix both issues at the same time.
Flags: needinfo?(ahunt)
Priority: -- → P1
I think it's not possible to run a fixup migration. Since I don't know what urls in uriannotations and bookmarks table are should treated as the same url (although there might me some pattern for medium.com).

I've sent out a patch. I defer saving url to DB for reader mode bookmarks. And use the same url to insert into both uriannotations and bookmarks table. And the bookmarks will be shown correctly now.
Flags: needinfo?(ahunt)
Comment on attachment 8826812 [details]
Bug 1317632 - If the bookmark to add is a reader mode page, defer db action till it cache completely and save the same url in bookmarks and uriannotations table to avoid URL chagne between two http requests.

https://reviewboard.mozilla.org/r/104694/#review105922

You work around this issue by first caching the item and then bookmarking the URL that was just cached. This makes this use case work - however did you ever find the reason for why the URL changes in the first place? This still seems wrong and in the worst case we are now using the "wrong" encoded URL as cache key and bookmark.
Attachment #8826812 - Flags: review?(s.kaspari) → review-
Comment on attachment 8826812 [details]
Bug 1317632 - If the bookmark to add is a reader mode page, defer db action till it cache completely and save the same url in bookmarks and uriannotations table to avoid URL chagne between two http requests.

https://reviewboard.mozilla.org/r/104694/#review106296

We talked on slack: Reader mode actually does load the page again. In this case it makes sense that the URL can cache and we'd have to bookmark the changed URL. So I r+ this patch to fix the issue.

Nevertheless it feels strange that after loading a page when clicking the reader mode button and bookmarking the page another load of the website is happening. Maybe Gijs knows if this is actually needed or a bug.

::: mobile/android/base/java/org/mozilla/gecko/Tab.java:8
(Diff revision 2)
> -import java.util.ArrayList;
> -import java.util.Collection;
> -import java.util.HashMap;
> -import java.util.Map;
> -import java.util.concurrent.Future;
> -import java.util.regex.Matcher;
> -import java.util.regex.Pattern;
> +import android.content.ContentResolver;
> +import android.content.Context;
> +import android.graphics.Bitmap;
> +import android.graphics.drawable.BitmapDrawable;
> +import android.os.Build;
> +import android.os.Bundle;
> +import android.text.TextUtils;
> +import android.util.Log;
> +import android.view.View;

nit: Avoid moving the imports in this patch. It's noise and leads to conflicts when uplifting patches. :)
Attachment #8826812 - Flags: review- → review+
(In reply to Sebastian Kaspari (:sebastian) from comment #15)
> In this case it makes sense that the URL can cache and we'd have to bookmark the
> changed URL.

This should be "[..] it makes sense that the URL can CHANGE [..]"
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e7c232eb0efa
If the bookmark to add is a reader mode page, defer db action till it cache completely and save the same url in bookmarks and uriannotations table to avoid URL chagne between two http requests. r=sebastian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e7c232eb0efa
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Tested with Huawei MediaPad M2 (Android 5.1.1) on Aurora 53.0a2 (2017-01-25) and the issue from description is still present. I have bookmarked two pages/articles from medium.com and one from Wikipedia, so I only have 3 pages saved offline but in description from my reading list from bookmark folder I have 5 items.
The fix wasn't landed? Thanks!
Did this happen with a completely fresh profile? We won't be able to recover from an already broken state afaik. Let's file a follow-up bug if this can still be reproduced.
Attachment #8826812 - Flags: review?(ahunt)
Flags: needinfo?(ahunt)
After further investigation seems that this bug is reproducible only if a have performed a sync first and after that I add articles to reading list. This is the same behavior on 52 Beta 2.
I couldn't manage to reproduce on a clean profile, with no sync performed.

Should I file a new bug?
(In reply to Sorina Florean [:sorina] from comment #22)
> Should I file a new bug?

Yes, please. Especially with sync in the STR this seems to have a different cause. Also let us know if there are any articles and/or medium URL in the synchronized profile.
See Also: → 1335415
Verified as fixed on Beta 53.0b7.
Devices:
-Asus ZenPad 8.0 Z380KL (Android 6.0.1)
-Huawei Honor 5X (Android 5.1.1)
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
See Also: → 1358946
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.