History remembers url instead of title for prematurely inserted lazy browsers

REOPENED
Unassigned

Status

()

defect
P2
normal
REOPENED
2 years ago
Last year

People

(Reporter: Oriol, Unassigned)

Tracking

({regression})

55 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 wontfix, firefox62 fix-optional)

Details

(Whiteboard: [fxsearch])

Attachments

(4 attachments)

Reporter

Description

2 years ago
Posted image screenshot.png
This does not always happen and I don't have a reliable way to reproduce it, but it happens lots of times.

1. Visit some page (allow it to be stored in history)
2. Close the tab, wait some time, open new tabs, restart Firefox, etc.
3. Enter some part of the title of that page

Result: if that text appears in the url, then the address bar suggests the page, but it shows the domain instead of the page title. If the text is only part of the title but does not appear in the url (e.g. this happens with bugzilla.mozilla.org bugs), then the page does not appear.

In the attached screenshot I searched "contain". I have https://drafts.csswg.org/css-contain/ in my history, but in the address bar it appeared as "drafts.csswg.org".

Then I navigated to that page, this temporarily solved the issue and right now I see "CSS Containment Module Level 1". But eventually it will break again.

Not sure if bug 1392652 is related, the tab title is briefly replaced with the url instead of the page title. Maybe this makes the address bar to remember the url instead of the title.

Not sure when this started, but at least versions 58 and 59 are affected, probably 57 too, maybe previous ones.
It's possible due to recent changes that the Docshell sometimes hands the wrong title to Global History. We don't have much control over that on the history side, we just store what we get passed.

If I try to load the url you posted, I see that first the tab shows the domain, and then later the final title, this may have changed recently indeed, earlier we were showing "Connecting..." IIRC.
Component: Address Bar → Document Navigation
Product: Firefox → Core
Reporter

Comment 2

2 years ago
Note https://drafts.csswg.org/css-contain/ was just an example, it can happen with any URL.
Reporter

Comment 3

2 years ago
Posted image screenshot2.png
See this other screenshot. When I write the title of a bugzilla bug, nothing is shown. But when I write the bug number, since it's what appears in the url, then it appears.
Reporter

Comment 4

2 years ago
Something that could also be related:
1. Enable automatic session restore
2. Load data:application/json,à
3. It is escaped into data:application/json,%C3%A0
4. Restart Firefox

Sometimes but now always the location bar shows data:application/json,à instead of data:application/json,%C3%A0
(In reply to Marco Bonardo [::mak] from comment #1)
> It's possible due to recent changes that the Docshell sometimes hands the
> wrong title to Global History. We don't have much control over that on the
> history side, we just store what we get passed.
> 
Samael, are you aware of recent changes that could cause this issue?
FWIW, I haven't been successful in reproducing this after tens of trials...

> If I try to load the url you posted, I see that first the tab shows the
> domain, and then later the final title, this may have changed recently
> indeed, earlier we were showing "Connecting..." IIRC.
Flags: needinfo?(sawang)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #5)
> (In reply to Marco Bonardo [::mak] from comment #1)
> > It's possible due to recent changes that the Docshell sometimes hands the
> > wrong title to Global History. We don't have much control over that on the
> > history side, we just store what we get passed.
> > 
> Samael, are you aware of recent changes that could cause this issue?
> FWIW, I haven't been successful in reproducing this after tens of trials...

Not that I'm aware of. Usually DocShell would first call History::VisitURI (on HttpChannel's OnStartRequest) to add the URI, and later when the title element gets parsed, it would update the title tho History::SetURITitle. I didn't find changes around these parts recently.

> > If I try to load the url you posted, I see that first the tab shows the
> > domain, and then later the final title, this may have changed recently
> > indeed, earlier we were showing "Connecting..." IIRC.

Yup look like it's changed in bug 1367630.
Flags: needinfo?(sawang)
I'm not sure where to start, but would you try to search the website in your full history (Ctrl+Shift+H) and see if the "Name" field of the website is actually empty, Oriol?
Reporter

Comment 8

2 years ago
(In reply to Samael Wang [:freesamael] from comment #7)
> I'm not sure where to start, but would you try to search the website in your
> full history (Ctrl+Shift+H) and see if the "Name" field of the website is
> actually empty, Oriol?

The Name is the url (without protocol) instead of the title.
(In reply to Oriol Brufau [:Oriol] from comment #8)
> The Name is the url (without protocol) instead of the title.

ah, that's correct for urls that don't have a title, and sort-of denies a problem in the docshell.
Since looks like the history entry has no title, I'm moving this back to Address Bar.
Component: Document Navigation → Address Bar
Product: Core → Firefox
Ok, found it:
https://searchfox.org/mozilla-central/source/toolkit/content/widgets/autocomplete.xml#2187
Looks like the behavior is by design.

Looks like sometimes the page is missing a title, and for pages without a title we use the domain.

Do you have something like "Clear history on shutdown" enabled?
Priority: -- → P3
Flags: needinfo?(oriol-bugzilla)
Reporter

Comment 11

2 years ago
(In reply to Marco Bonardo [::mak] from comment #10)
> Ok, found it:
> https://searchfox.org/mozilla-central/source/toolkit/content/widgets/
> autocomplete.xml#2187
> Looks like the behavior is by design.
> 
> Looks like sometimes the page is missing a title, and for pages without a
> title we use the domain.
> 
> Do you have something like "Clear history on shutdown" enabled?

No, I don't clear history. Note that in the screenshot in comment 8, all three entries are the same page, which has a title, and only the hash varies. However, the first entry does not have that title. So it seems the problem is that sometimes the title is not recognized or is forgotten.
Flags: needinfo?(oriol-bugzilla)
different refs are different uris, for Places at least.  And it's impossible that a title gets "forgotten" if the page is still there, because titles and pages are removed at once.

The only explanation I could give is that somehow the docshell may not have invoked setURITitle for one of the refs (sorry for the ping pong between components).
The Places situation from what I see is clear, that specific uri has no title, the other 2 have. Since a title cannot "disappear", either it was not set at all, or something updated it to an empty string.

By chance, is any of those 3 uris bookmarked?
Reporter

Comment 13

2 years ago
(In reply to Marco Bonardo [::mak] from comment #12)
> Since a title cannot "disappear", either it was not
> set at all, or something updated it to an empty string.

Probably the latter, because when I navigate to a page with that problem then it's temporarily fixed.

> By chance, is any of those 3 uris bookmarked?

Nope.
I wonder if it could be Sync setting an empty history title through history.insertMany.
Do you have Sync enabled?

By a quick look at History.cpp, we seem to always override title when a new visit arrives, even if the provided title is null. We should probably not. The only risk I see, is that if a page has a title, and then suddenly stops having a title, we'd retain the old title forever.

Kit, do you know if Sync is properly passing a null title, rather than an empty string, when syncing visits? If that's true, we should be able to fix history to skip updating title if the new one is null.
Component: Address Bar → Places
Flags: needinfo?(kit)
Priority: P3 → P2
Product: Firefox → Toolkit
Whiteboard: [fxsearch]
Reporter

Comment 15

2 years ago
I don't have Sync enabled.
That is puzzling! but the History API overwriting a title with null is still a good candidate and looks like a bug. We just have to find the caller.

Do you have an add-on that may add visits/pages to history?

Otherwise, I'm curiously looking into this code
https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/browser/extensions/activity-stream/lib/SectionsManager.jsm#175
https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/browser/extensions/activity-stream/lib/SectionsManager.jsm#182
Mardak, is there a way to avoid adding a fake visit? when does that happen? what is card.title usually? Why are we setting a title at all? (sorry for the many questions, I don't know that code)
Flags: needinfo?(edilee)
Reporter

Comment 17

2 years ago
No, I don't have any history-related add-on.

Comment 18

2 years ago
mak, that code should only be called when a user bookmarks a pocket story. The visit was added to have it appear different enough from default bookmarks that are not visited. Typically, the titles of these cards are the cleaned up article title (e.g., "New in Firefox 58: Developer Edition" `<meta property="og:title" content="New in Firefox 58: Developer Edition – Mozilla Hacks - the Web developer blog">`) although in reality these come from Pocket service, so they might be doing some other types of cleanups to get the title.
Flags: needinfo?(edilee)
Reporter

Comment 19

2 years ago
Then it's something else, because I disabled Pocket.
Apart from Migration, Add-ons, Activity Stream and Sync, I cannot find anything else in our code that uses history.insert*. Something is surely overwriting the title though.
Reporter

Comment 21

2 years ago
I also disabled Activity Stream, by the way.

I opened the History sidebar and kept F5 key pressed down to reload a page that only has this HTML:

    <title>Hello</title>

I could see that for a tiny moment, the history entry became localhost/test/ instead of Hello. Probably this is bug 1392652.

So what I suspect is that, when I restore the session, something like bug 874533 may speculatively start a connection, and this is stored in history before the title is received, overwriting the old good title with an empty one. The code added in bug 874533 throws an error for me (bug 1418009), so it might be behaving incorrectly.
(In reply to Oriol Brufau [:Oriol] from comment #21)
> So what I suspect is that, when I restore the session, something like bug
> 874533 may speculatively start a connection, and this is stored in history
> before the title is received, overwriting the old good title with an empty
> one.

That would be wrong, we should not be storing a visit for a speculative connection, the only thing that stores visits is the docshell. If we did, you'd see 2 entries in history sidebar when set by Last Visited. Do you see that?
I'll file a separate bug to fix History overriding a page title and make it block this one that should continue investigating the callers.
Depends on: 1419825
Reporter

Comment 23

2 years ago
No, I don't see double entries, but only the last visit per url is shown, isn't it?
(In reply to Oriol Brufau [:Oriol] from comment #23)
> No, I don't see double entries, but only the last visit per url is shown,
> isn't it?

Yes, you are right, looks like we changed this and I forgot.

I have a patch fixing the "history overwriting title with empty title" problem that may help. though from that point on it all depends on the consumers to do the right thing, that is passing null or undefined as title if the title shouldn't change, rather than an empty string.
It sounds like Sync isn't causing this, since Oriol isn't using Sync, but...

(In reply to Marco Bonardo [::mak] from comment #14)
> Kit, do you know if Sync is properly passing a null title, rather than an
> empty string, when syncing visits? If that's true, we should be able to fix
> history to skip updating title if the new one is null.

Desktop passes null when it uploads history visits, yes; it just uses whatever's in `moz_places` to inflate the record: https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/toolkit/components/places/PlacesSyncUtils.jsm#223

However, it looks like iOS has a NOT NULL constraint on the title (https://github.com/mozilla-mobile/firefox-ios/blob/345112875c657e2f959d9a0ea4876dc1a4f2d875/Storage/SQL/BrowserSchema.swift#L620), so I'm guessing we might end up with empty strings if we insert a page synced from iOS. Android is nullable (https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java#165), but I don't know if it might upload records with empty title strings, too.

Should we change Sync to replace empty strings with `null` when it passes page infos to `PlacesUtils.history.insertMany`?
Flags: needinfo?(kit)
(In reply to Kit Cambridge (he/him) [:kitcambridge] from comment #25)
> Should we change Sync to replace empty strings with `null` when it passes
> page infos to `PlacesUtils.history.insertMany`?

Bug 1419825 answered my question. :-) No, we shouldn't.
The next Nightly should (hopefully) contain the fix for bug 1419825, and then it would be useful to know if this still reproduces.
Flags: needinfo?(oriol-bugzilla)
Reporter

Comment 28

2 years ago
Effectively it seems titles are no longer disappearing, but hard to tell because I didn't have reliable steps to reproduce the issue. And of course the history entries that lost the title before bug 1419825 have not magically recovered it. But navigating to them seems a permanent fix.
Flags: needinfo?(oriol-bugzilla)
let's consider this fixed, and if you can reproduce again in the next days you can reopen this, thank you!
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Reporter

Comment 30

2 years ago
Reopening because I visited this bug after bug 1419825 in order to post comment 28, but the bug title does not appear in history.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter

Comment 31

2 years ago
Keywords: steps-wanted
Priority: P2 → P3
Reporter

Comment 32

2 years ago
So it seems the title becomes the empty string instead of null.
Then I added an overlay legacy extension with this code:

var obj = PlacesUtils.history;
PlacesUtils.history = new Proxy(obj, {get(target, prop, receiver) {
  let v = Reflect.get(target, prop, receiver);
  if (typeof v == "function") {
    return function(...args) {
      console.log(prop, ...args);
      if (args.length && args[0].title === "") {
        alert("title is empty string");
        throw new Error("title is empty string");
      }
      return new.target ? Reflect.construct(v, args, new.target) : Reflect.apply(v, this, args);
    };
  }
  return v;
}});

I expected this would detect any use of PlacesUtils.history.insert or PlacesUtils.history.update that would set the title to the empty string. But the problem occurred again and I didn't see any alert. I just get lots of logs when I navigate.

Any idea?
(In reply to Oriol Brufau [:Oriol] from comment #32)
> I expected this would detect any use of PlacesUtils.history.insert or
> PlacesUtils.history.update that would set the title to the empty string. But
> the problem occurred again and I didn't see any alert. I just get lots of
> logs when I navigate.

The docshell uses the IHistory API and goes through History.cpp directly, so mocking PlacesUtils.history won't help, since it's a wrapper around History.cpp. The entry points in History.cpp are UpdatePlaces, VisitURI and SetURITitle.
Reporter

Comment 34

Last year
It seems this has not been happening lately. I believe the problem was that I had a legacy add-on which caused premature insertion of lazy browsers at startup. I suspect this was treated as a visit but the page was not really fetched unless I activated the tab, and thus the title was not known. Will try if I can confirm by inserting lazy browsers manually.
Reporter

Comment 35

Last year
Effectively, steps to reproduce:

1. Enable automatic session restore
2. Enable devtools.chrome.enabled
3. Open a new window with two tabs
4. In the first tab, load https://bugzilla.mozilla.org/show_bug.cgi?id=1418038
5. Switch to the second tab
6. Restart Firefox
7. You may open History and check that the title is still there
8. Open the browser console (Ctrl+Shift+J) in the window with the two tabs
   (if it's already open it may refer to another window, so close it first)
9. Enter this code:
   gBrowser.tabs[0].linkedBrowser.addProgressListener

You will get "Lazy browser prematurely inserted via 'addProgressListener' property access".
And simultaneously, the title will disappear from history.


Marco, is this enough to detect what is removing the title?
Flags: needinfo?(mak77)
Reporter

Comment 36

Last year
Regression window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=70d524455cdaa4ef1ee2980f878ee50d9be69c1e&tochange=4180fb6074b11fa29119cdef6a90432be01f0036

Mike, any idea?
Blocks: 1370035
Has Regression Range: --- → yes
Has STR: --- → yes
Flags: needinfo?(mconley)
Keywords: steps-wantedregression
Summary: Address bar sometimes remembers url instead of title → History remembers url instead of title for prematurely inserted lazy browsers
Version: unspecified → 55 Branch
I can reproduce the "Lazy browser prematurely inserted" error, but I don't see the title disappearing :(
I was hoping to set breakpoints in SetURITitle and VisitURI in History.cpp to get the calling stacks when the title is set to empty, but they are not invoked at all for me. Indeed the title stays in history.
Either something in Nightly has resolved this or it only reproduces on some OS.
Maybe Mike will have more luck reproducing it...

I wonder if there's any missing details, like when you say "Restart Firefox", how you do that? On which Firefox version? On which OS?
Reporter

Comment 38

Last year
"Restart Firefox" means clicking "Exit" in the menu and then clicking a link that points to firefox.exe
I can reproduce on latest 64-bit nightly 2018-03-15 rev 68dfe5ee5b80ee99b7e389d739a30089f6f1e55d
I have Windows 10 pro 1709 16299.248 64-bit.

I will try compiling a debug build myself.
Reporter

Comment 39

Last year
I can't attach the debugger to the firefox process (Unrecognized error occurred in the Windows Web Services framework.)

But I noticed that the problem only occurs when e10s is disabled (browser.tabs.remote.autostart = false)
Reporter

Comment 40

Last year
I managed to make the debugger work.
There is a History::SetURITitle call where aURI contains "https://bugzilla.mozilla.org/show_bug.cgi?id=1418038" and aTitle's mLength is 0.
Call stack:


	xul.dll!mozilla::places::History::SetURITitle(nsIURI * aURI, const nsTSubstring<char16_t> & aTitle) Line 2988	C++
 	xul.dll!nsDocShell::UpdateGlobalHistoryTitle(nsIURI * aURI) Line 14229	C++
 	xul.dll!nsDocShell::SetTitle(const nsTSubstring<char16_t> & aTitle) Line 6207	C++
 	xul.dll!nsIDocument::DoNotifyPossibleTitleChange() Line 6507	C++
 	xul.dll!mozilla::detail::RunnableMethodArguments<>::applyImpl<nsIDocument,void (__cdecl nsIDocument::*)(void) __ptr64>(nsIDocument * o, void(nsIDocument::*)() m, mozilla::Tuple<> & args, mozilla::IndexSequence<> __formal) Line 1150	C++
 	xul.dll!mozilla::detail::RunnableMethodArguments<>::apply<nsIDocument,void (__cdecl nsIDocument::*)(void) __ptr64>(nsIDocument * o, void(nsIDocument::*)() m) Line 1157	C++
 	xul.dll!mozilla::detail::RunnableMethodImpl<nsIDocument * __ptr64,void (__cdecl nsIDocument::*)(void) __ptr64,0,0>::Run() Line 1203	C++
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 1100	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 517	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate) Line 97	C++
 	xul.dll!MessageLoop::RunInternal() Line 327	C++
 	xul.dll!MessageLoop::RunHandler() Line 320	C++
 	xul.dll!MessageLoop::Run() Line 300	C++
 	xul.dll!nsBaseAppShell::Run() Line 159	C++
 	xul.dll!nsAppShell::Run() Line 401	C++
 	xul.dll!nsAppStartup::Run() Line 290	C++
 	xul.dll!XREMain::XRE_mainRun() Line 4724	C++
 	xul.dll!XREMain::XRE_main(int argc, char * * argv, const mozilla::BootstrapConfig & aConfig) Line 4869	C++
 	xul.dll!XRE_main(int argc, char * * argv, const mozilla::BootstrapConfig & aConfig) Line 4961	C++
 	xul.dll!mozilla::BootstrapImpl::XRE_main(int argc, char * * argv, const mozilla::BootstrapConfig & aConfig) Line 50	C++
 	firefox.exe!do_main(int argc, char * * argv, char * * envp) Line 232	C++
 	firefox.exe!NS_internal_main(int argc, char * * argv, char * * envp) Line 304	C++
 	firefox.exe!wmain(int argc, wchar_t * * argv) Line 129	C++
Reporter

Comment 41

Last year
The title is the empty string because there is no <title> element:
https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/dom/base/nsDocument.cpp#6394-6397

Effectively, in the browser console I can run

  gBrowser.tabs[0].linkedBrowser.contentDocument.documentElement.outerHTML

and it's just

  "<html><head></head><body></body></html>"

Would it be possible to return null instead of empty string if there is no <title>, and then don't overwrite?
that seems to suggest we add to history a visit that should not be added, if the dom is not even the real one.
The bug is reproducible even with just a single window, just have a window with the bug page open and a blank tab selected as second tab, set browser.tabs.remote.autostart to false, restart the browser and in the browser console use gBrowser.tabs[0].linkedBrowser.addProgressListener. Library history will show the url instead of the real title.

I suspect we insert an empty document when the "Lazy browser prematurely inserted" happens, and if that is considered real by the docshell, it will parse it and try to set the title. Mike knows far more than me about that. Though, now that I can reproduce I can also try to debug.
And the setURITitle protection I added in bug 1419825 in this case doesn't work because setURITitle is invoked with an empty string, instead of a void string.
In particular DoNotifyPossibleTitleChange() creates an autoString, gets the title (early return after a Truncate()) and passes it to the docshell. There's no distinction between empty title or no <title> tag. 
Anyway, this should not be notified to the docshell at all, we should set something on the docshell so that NotifyPossibleTitleChange() is skipped for this case.
in the docshell mIsActive is false, I wonder if we could use this information to skip all (or part of) SetTitle.
I'm actually going to redirect this to dao - I think he has a better sense of how the lazy tabs framework works. Unfortunately, it appears that Kevin Jones is no longer active on Bugzilla, otherwise I'd ask him. :/
Flags: needinfo?(mconley) → needinfo?(dao+bmo)
(In reply to Marco Bonardo [::mak] from comment #33)
> (In reply to Oriol Brufau [:Oriol] from comment #32)
> > I expected this would detect any use of PlacesUtils.history.insert or
> > PlacesUtils.history.update that would set the title to the empty string. But
> > the problem occurred again and I didn't see any alert. I just get lots of
> > logs when I navigate.
> 
> The docshell uses the IHistory API and goes through History.cpp directly, so
> mocking PlacesUtils.history won't help, since it's a wrapper around
> History.cpp. The entry points in History.cpp are UpdatePlaces, VisitURI and
> SetURITitle.

Does setCurrentURI end up triggering any of these things?

https://searchfox.org/mozilla-central/rev/b29daa46443b30612415c35be0a3c9c13b9dc5f6/browser/components/sessionstore/ContentRestore.jsm#156
Flags: needinfo?(dao+bmo)
Reporter

Comment 48

Last year
(In reply to Dão Gottwald [::dao] from comment #47)
> Does setCurrentURI end up triggering any of these things?

Commenting out that line seems to fix the issue, yes!
(In reply to Oriol Brufau [:Oriol] from comment #48)
> Commenting out that line seems to fix the issue, yes!

Dão, does this mean it's something actionable for you?
Flags: needinfo?(dao+bmo)
(In reply to Mike de Boer [:mikedeboer] (Back! Processing backlog...) from comment #49)
> (In reply to Oriol Brufau [:Oriol] from comment #48)
> > Commenting out that line seems to fix the issue, yes!
> 
> Dão, does this mean it's something actionable for you?

Maybe... It might be a good idea to just get rid of setCurrentURI, as that's underutilized with lazy browsers and is likely to cause more trouble in the future.
Flags: needinfo?(dao+bmo)
Flags: needinfo?(mak77)
Priority: P3 → P2
You need to log in before you can comment on or make changes to this bug.