Closed Bug 751650 Opened 10 years ago Closed 1 year ago

Favicons/titles not updated after showing error pages

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(blocking-fennec1.0 soft)

RESOLVED INCOMPLETE
Tracking Status
blocking-fennec1.0 --- soft

People

(Reporter: bnicholson, Unassigned)

References

Details

(Whiteboard: ui-hackathon)

Attachments

(3 files)

When trying to reproduce bug 740565, I came across a similar bug where redirected pages don't get updated.

STR:
1) Put phone in airplane mode
2) Load some page that redirects (e.g., yahoo.com or microsoft.com)
3) After the "Server not found" page appears, turn off airplane mode
4) Visit the site in step 2.

After the page loads in step 4, the redirected page still shows up in the AwesomeScreen with the title "Problem loading page" and an error icon.

There's some overlap here with bug 731370 since removing these redirected entries would make this bug a non-issue.
Nom'ing for soft block.
blocking-fennec1.0: --- → ?
Assignee: nobody → bnicholson
blocking-fennec1.0: ? → +
I've noticed that is also reproducible on desktop, but I suppose it's not as big of a deal there since 1) network connectivity is generally more stable, and 2) there are fewer redirections.
We currently send the documentURI in onLocationChange, but this is too early, and the documentURI refers to the previous page. Waiting until the document is created fixes this.
Attachment #620878 - Flags: review?(mark.finkle)
When we have error pages, the documentURI is different from the requested URL. We can use this to determine whether the favicon/title should be saved to the database.
Attachment #620880 - Flags: review?(mark.finkle)
This is technically a separate bug that became obvious when making these fixes. We update the page's title/URL in the database (when we call tab.updateURL()) before we get a DOMTitleChanged event. This means mTitle in the Tab is still set to the title from the previous page, and the wrong title gets saved to the database - although it's quickly fixed when we get the DOMTitleChanged event.

This patch resets mTitle to the page's URL in onLocationChange.
Attachment #620882 - Flags: review?(mark.finkle)
Comment on attachment 620878 [details] [diff] [review]
Part 1: Send documentURI when document is created

I have a ringing in the back of my head whispering to me that adding more JS/Java messages could affect performance. I'll let this go, but I wonder if DOMContentLoaded or pageshow would also work. When are they fired relative to the "content-document-global-created" notification?

If "content-document-global-created" is the best place to send it (and it might be) then so be it.
Attachment #620878 - Flags: review?(mark.finkle) → review+
Comment on attachment 620880 [details] [diff] [review]
Part 2: Only update favicon and title if documentURI matches URL

># HG changeset patch

>diff --git a/mobile/android/base/Favicons.java b/mobile/android/base/Favicons.java

>     private class LoadFaviconTask extends AsyncTask<Void, Void, BitmapDrawable> {

>+        private boolean mSaveToDb;
> 
>-        public LoadFaviconTask(String pageUrl, String faviconUrl, OnFaviconLoadedListener listener) {
>+        public LoadFaviconTask(String pageUrl, String faviconUrl, boolean saveToDb, OnFaviconLoadedListener listener) {

I find myself wishing we didn't need to pass in the "saveToDb" param, but I don't see any way around it with our current code.

>diff --git a/mobile/android/base/Tab.java b/mobile/android/base/Tab.java

>+    public void updateTitle(String title, boolean saveToDb) {

The Tab actually has both the URL and the documentURI. Can we just check them in this function and not require a boolean? I don't like requiring an extra param if we can do the check externally or, as in thiis case, internally.

Is there any reason we couldn't do the check internally?

r- for the updateTitle change
Attachment #620880 - Flags: review?(mark.finkle) → review-
Comment on attachment 620882 [details] [diff] [review]
Part 3: Reset page title onLocationChange

      
>-        tab.updateURL(uri);
>-
>         if (sameDocument) {
>+            tab.updateURL(uri);

>             return;
>         }
> 
>+        tab.updateTitle(uri, false);
>+        tab.updateURL(uri);

Why move tab.updateURL(uri) ?
Attachment #620882 - Flags: review?(mark.finkle) → review-
(In reply to Mark Finkle (:mfinkle) from comment #6)
> Comment on attachment 620878 [details] [diff] [review]
> Part 1: Send documentURI when document is created
> 
> I have a ringing in the back of my head whispering to me that adding more
> JS/Java messages could affect performance. I'll let this go, but I wonder if
> DOMContentLoaded or pageshow would also work. When are they fired relative
> to the "content-document-global-created" notification?
> 
> If "content-document-global-created" is the best place to send it (and it
> might be) then so be it.

I was originally going to go with DOMContentLoaded, but it's fired after DOMTitleChanged. I found that pageshow didn't get fired when showing error pages, so that doesn't seem to be an option either. "content-document-global-created" is fired before any of the other DOM events, and it's fired on error pages, so it seems to work well for this case.
(In reply to Brian Nicholson (:bnicholson) from comment #9)
> (In reply to Mark Finkle (:mfinkle) from comment #6)
> > Comment on attachment 620878 [details] [diff] [review]
> > Part 1: Send documentURI when document is created
> > 
> > I have a ringing in the back of my head whispering to me that adding more
> > JS/Java messages could affect performance. I'll let this go, but I wonder if
> > DOMContentLoaded or pageshow would also work. When are they fired relative
> > to the "content-document-global-created" notification?
> > 
> > If "content-document-global-created" is the best place to send it (and it
> > might be) then so be it.
> 
> I was originally going to go with DOMContentLoaded, but it's fired after
> DOMTitleChanged. I found that pageshow didn't get fired when showing error
> pages, so that doesn't seem to be an option either.
> "content-document-global-created" is fired before any of the other DOM
> events, and it's fired on error pages, so it seems to work well for this
> case.

OK, let's go the other direction. Can we drop DOMContentLoaded and use "content-document-global-created"? Just checking my options :)
It would have to match DOMContentLoaded's behavior of not being fired for back/forward cahced pages.
(In reply to Mark Finkle (:mfinkle) from comment #7)
> Comment on attachment 620880 [details] [diff] [review]
> Part 2: Only update favicon and title if documentURI matches URL
> 
> >diff --git a/mobile/android/base/Tab.java b/mobile/android/base/Tab.java
> 
> >+    public void updateTitle(String title, boolean saveToDb) {
> 
> The Tab actually has both the URL and the documentURI. Can we just check
> them in this function and not require a boolean? I don't like requiring an
> extra param if we can do the check externally or, as in thiis case,
> internally.
> 
> Is there any reason we couldn't do the check internally?
> 
> r- for the updateTitle change

There's two different cases where we don't want to update the db in updateTitle: 1) documentURI != URL, and 2) when resetting the page's title (added in patch 3). I added the boolean since there was no way to internally determine case #2.
(In reply to Mark Finkle (:mfinkle) from comment #8)
> Comment on attachment 620882 [details] [diff] [review]
> Part 3: Reset page title onLocationChange
> 
>       
> >-        tab.updateURL(uri);
> >-
> >         if (sameDocument) {
> >+            tab.updateURL(uri);
> 
> >             return;
> >         }
> > 
> >+        tab.updateTitle(uri, false);
> >+        tab.updateURL(uri);
> 
> Why move tab.updateURL(uri) ?

updateURL() updates the db with the current mUrl and mTitle. We can't update this URL yet until we've set mTitle, or else we'd be updating with the previous page's title.
(In reply to Brian Nicholson (:bnicholson) from comment #12)
> (In reply to Mark Finkle (:mfinkle) from comment #7)
> > Comment on attachment 620880 [details] [diff] [review]
> > Part 2: Only update favicon and title if documentURI matches URL
> > 
> > >diff --git a/mobile/android/base/Tab.java b/mobile/android/base/Tab.java
> > 
> > >+    public void updateTitle(String title, boolean saveToDb) {
> > 
> > The Tab actually has both the URL and the documentURI. Can we just check
> > them in this function and not require a boolean? I don't like requiring an
> > extra param if we can do the check externally or, as in thiis case,
> > internally.
> > 
> > Is there any reason we couldn't do the check internally?
> > 
> > r- for the updateTitle change
> 
> There's two different cases where we don't want to update the db in
> updateTitle: 1) documentURI != URL, and 2) when resetting the page's title
> (added in patch 3). I added the boolean since there was no way to internally
> determine case #2.

OK. I need to think about this. I really don't want to start adding "saveToDB" params to our property setters.
(In reply to Mark Finkle (:mfinkle) from comment #10)
> 
> OK, let's go the other direction. Can we drop DOMContentLoaded and use
> "content-document-global-created"? Just checking my options :)

I don't think so - we do things like this in DOMContentLoaded:

    final String backgroundColor = message.getString("bgColor");

which wouldn't be available upon document creation.
Another hook you may find useful is the setFirstPaintViewport call in GeckoLayerClient. That gets called when page being displayed to the user changes, either because a new page loaded and we just started drawing it, or because of tab switches. If necessary you can use that hook to throw something on the UI thread for UI updates, so long as you don't modify any of the existing code paths :)
Brian - Does this bug depend on some other refactor work in a different bug?
(In reply to Mark Finkle (:mfinkle) from comment #17)
> Brian - Does this bug depend on some other refactor work in a different bug?

Kind of...we should split updateTitle() and updateURL() so that the saving to DB is done in a separate method to address comment 12 and comment 13. This can be done without any of the refactoring in bug 753217, but that bug should make it cleaner (since it moves this logic inside of Tab).
I'd really like to see this fixed, (and to approve that fix!) but it's not actually a blocker at this point. We could ship without the fix, if this were the last thing.
blocking-fennec1.0: + → soft
Brian - Can we attempt any fix without needing a large refactor? If not, I'd just say we just - this bug and wait for the refactor.
(In reply to Mark Finkle (:mfinkle) from comment #20)
> Brian - Can we attempt any fix without needing a large refactor? If not, I'd
> just say we just - this bug and wait for the refactor.

The patches posted here fix the bug without refactoring. I don't think we can make it much cleaner without moving the logic to Tab's internals.
Duplicate of this bug: 822815
Whiteboard: ui-hackathon
Does this bug still exist? We don't save redirects (or error pages) anymore.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
After turning off the "Airplane mode", the page reloads correctly and the title "Problem loading page" and an error icon don't appear anymore.
Verified fixed on:
Device: Samsung Galaxy Nexus
OS: Android 4.1.1
Build: Firefox for Android 24.0a1(2013-06-11)
Sounds like we still have some issues updating error pages. From Josh Aas:

'I use FF exclusively on NY Android phone. Some time ago (6 months?) I had a bad internet connection and the title of the page returned for Hacker News was something like "Error 500 Bad Gateway". That title has persisted, so whenever I see the new tab options that is the top option text. I would expect it to update to the correct title ("Hacker News").'

I was able to reproduce something similar to this myself by loading a page on people.mozilla.org, setting a rewrite rule to return an HTTP 500, then loading the page again. The 500 error title gets saved since the page already exists in history. We should fix this by not updating titles/favicons for pages that don't return a 200.

Also, we'll see the same problem when behind captive portals (which I noticed recently when I was out of the country). These pages often return an HTTP 200, however, so I'm not sure we could detect being behind a captive portal without pinging some service.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: bnicholson → nobody
Summary: Favicons/titles not updated for redirected pages → Favicons/titles not updated after showing error pages
This error is also present on desktop. I think it may be bug 489186?
The error from Josh seems to be that we weren't updating the title when he revisited the site. I sorta suspect that might be due to a redirect that we were (incorrectly) adding to our history before and then never fixing?

Based on my current tests, we do update page titles when you go back to the page. I'm going to mark this as WFM, and we can fix the platform in bug 489186. Reopen if you can reproduce what Josh was seeing.
Status: REOPENED → RESOLVED
Closed: 9 years ago8 years ago
Resolution: --- → WORKSFORME
I have Firefox 23 on my Galaxy Nexus with Android 4.3. About a year ago I got bad gateway error visiting Hacker News on this phone and it has showed that title ever since. It does not update when re-visiting the site.

The only tricky thing I can think of is that the URL for the site in my top sites is "http://...", and it re-directs to "https://...", so maybe it updates the info for the https version but the http stays bad gateway.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
There's other situations that could cause this to happen. Say some page is in history, and the page is later moved and redirects to somewhere else -- that then becomes an in-history redirecting page.

But if we don't save titles/favicons in history to begin with (which is what the patches in this bug were for), we don't have to worry about any of this. That's now bug 840989.

Maybe this one can be morphed to address Josh's issue, which is to purge error favicons/titles that have already been stored.
Josh, can you delete the old entry (long click on it and hit remove?) and see if the problem is fixed.

We could send a "delete history entry" command whenever we hit redirects to fix people who have old histories... I'm surprised that history entry has lasted a year though. Bug 788073 will fix the favicons, but needs some similar logic to fixup old profiles. I can try to tackle both here.
See Also: → 489186
Hardware: ARM → All
Duplicate of this bug: 1280269
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: REOPENED → RESOLVED
Closed: 8 years ago1 year ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.