HTML <link> insertion for Fluent trips history link modification watching which trips history initialization which doesn't work without a profile
Categories
(Core :: Internationalization, enhancement)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox68 | --- | fixed |
People
(Reporter: Gijs, Assigned: Gijs)
References
Details
Attachments
(1 file, 1 obsolete file)
Unfortunately for us, HTML has several things that qualify as "links".
There are (at least?) <a>, <area> and <link> tags, which all inherit from Link in our codebase.
Fluent uses <link rel="localization"> to keep track of localization files.
Places uses link state callbacks to keep track of history state:
#01: mozilla::places::History::GetIsVisitedStatement(mozIStorageCompletionCallback*)[dist/Nightly.app/Contents/MacOS/XUL +0x39d8273]
#02: mozilla::places::(anonymous namespace)::VisitedQuery::Start(nsIURI*, mozIVisitedStatusCallback*)[dist/Nightly.app/Contents/MacOS/XUL +0x39dafd3]
#03: mozilla::places::History::RegisterVisitedCallback(nsIURI*, mozilla::dom::Link*)[dist/Nightly.app/Contents/MacOS/XUL +0x39dac50]
#04: mozilla::dom::Link::LinkState() const[dist/Nightly.app/Contents/MacOS/XUL +0x1135d29]
#05: mozilla::dom::Document::FlushPendingLinkUpdates()[dist/Nightly.app/Contents/MacOS/XUL +0x10ff6b2]
#06: mozilla::detail::RunnableMethodImpl<mozilla::dom::Document*, void (mozilla::dom::Document::*)(), true, (mozilla::RunnableKind)0>::Run()[dist/Nightly.app/Contents/MacOS/XUL +0x1130837]
#07: IdleRunnableWrapper::Run()[dist/Nightly.app/Contents/MacOS/XUL +0xf50c7]
#08: nsThread::ProcessNextEvent(bool, bool*)[dist/Nightly.app/Contents/MacOS/XUL +0xeadc3]
#09: nsThreadManager::SpinEventLoopUntilInternal(nsINestedEventLoopCondition*, bool)[dist/Nightly.app/Contents/MacOS/XUL +0xecfc7]
In bug 1518234 we're trying to use fluent in migration.xul, which runs prior to profile setup if invoked using the -migration commandline flag, which also happens as part of Firefox Reset/Refresh.
Unfortunately, that breaks things. Rather badly, in fact. Because places initializes and assumes that nobody is stupid enough to initialize it before there is a profile. If people do try and do so, things break. More specifically, the history component will just assume that everything is broken and fail all database connections for the rest of Firefox's uptime.
This manifests, if you apply the patch from bug 1518234, by the migration never completing. This is because it tries to do some initial bookmark imports and then initialize places, right after it forces profile startup (which would make the above profile directory available etc.). Of course, at this point it's too late and the database is corrupt. But the code will bail out and never send a "places initialization is finished" notification which means that the migration code waits forever for one of those notifications, and so the dialog hangs forever.
So there's a few different things to consider in terms of fixing this... pile of unfortunate coincidences:
- fix HTML to not have so many things called "link", because it's clearly a liability/design flaw. I'm told this is not an option.
- make places' initialization code return early if called before we have a profile, and just no-op somehow (allowing us to retry later).
- make fluent's <link>s not trigger history visited state thingies. I dunno what trips this, but it seems like a waste of time either way.
I think we probably need both 2/3, though I guess for the moment fixing either one should be sufficient to unblock 1518234. Marco, is there anything that would stop us from doing (2)? Gandalf, same question for (3) ?
|
||
Comment 1•6 years ago
|
||
I believe there's nothing blocking us from that. I'm even surprised that a generic link causes visited state changes...
I'm going to loop in smaug in case I'm missing something obvious.
|
||
Comment 2•6 years ago
|
||
History should only care about <a> links, other inherited entries imo should skip RegisterVisitedCallback, so to me it looks like a bug in the Link implementation. Unless there's a compelling reason for which linkState matters for <link>.
Maybe in the Link constructor we may check what is mElement and set mHistory accordingly rather than assuming it's always true, or we could have a Link(mElement, aUseHistory) constructor for a cleaner approach, then it's a decision of the inherited class.
Also, I don't know which document is this one, but we could disable global history in its docshell setting useGlobalHistory to false.
Regarding history singleton, we likely bail out too late, so the singleton has already set the static ref to the history service, we could make the constructor bailout on a missing profile before gService is set (https://searchfox.org/mozilla-central/source/toolkit/components/places/History.cpp#1448) and the same for nsNavHistory::nsNavHistory. Though, this means that repeated calls would try to init these services pointlessy, wasting resources. I'd personally consider this a bug in the caller.
|
|
||
Comment 3•6 years ago
|
||
Though, I suspect useGlobalHistory doesn't really disable link coloring.
|
Assignee | |
Comment 4•6 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #2)
History should only care about <a> links, other inherited entries imo should skip RegisterVisitedCallback, so to me it looks like a bug in the Link implementation. Unless there's a compelling reason for which linkState matters for <link>.
Because the HTML and CSS spec say <link> is also a hyperlink and also has a :visited state. So we'd probably break webcompat if we broke all of this for <link>.
Also, I don't know which document is this one, but we could disable global history in its docshell setting useGlobalHistory to false.
(In reply to Marco Bonardo [::mak] from comment #3)
Though, I suspect useGlobalHistory doesn't really disable link coloring.
Well, that is unfortunate...
|
|
||
Comment 5•6 years ago
|
||
(In reply to :Gijs (he/him) from comment #4)
(In reply to Marco Bonardo [::mak] from comment #2)
History should only care about <a> links, other inherited entries imo should skip RegisterVisitedCallback, so to me it looks like a bug in the Link implementation. Unless there's a compelling reason for which linkState matters for <link>.
Because the HTML and CSS spec say <link> is also a hyperlink and also has a :visited state. So we'd probably break webcompat if we broke all of this for <link>.
I'm not sure I can think of a use-case for visit status of <link>, and the brokeness would be limited to just always reporting it as unvisited.
Fwiw, I'd be in favor of making useGlobalHistory also disable link coloring, though someone more expert regarding that part of the code should evaluate that. I think in this case we are looking at a document that doesn't want any history hints, nor registering nor caring about visited status.
|
|
Assignee | |
Comment 6•6 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #5)
(In reply to :Gijs (he/him) from comment #4)
(In reply to Marco Bonardo [::mak] from comment #2)
History should only care about <a> links, other inherited entries imo should skip RegisterVisitedCallback, so to me it looks like a bug in the Link implementation. Unless there's a compelling reason for which linkState matters for <link>.
Because the HTML and CSS spec say <link> is also a hyperlink and also has a :visited state. So we'd probably break webcompat if we broke all of this for <link>.
I'm not sure I can think of a use-case for visit status of <link>, and the brokeness would be limited to just always reporting it as unvisited.
Yes, but it's web-exposed and I'm not taking bets on what the web does/doesn't do.
Fwiw, I'd be in favor of making useGlobalHistory also disable link coloring, though someone more expert regarding that part of the code should evaluate that. I think in this case we are looking at a document that doesn't want any history hints, nor registering nor caring about visited status.
Yeah, except it seems all chrome window documents have useGlobalHistory set to false, and I expect we do ideally want link colouring when it's available (ie when there's a profile). And there'd be no way to re-enable it except by turning on useGlobalHistory on the docshell, which presumably has other side-effects.
|
|
||
Comment 7•6 years ago
|
||
Well, if there's no other possibility, I guess the only remaining solution is to bailout early (basically immediately) from the History constructor if a profile doesn't exist.
I'd still suggest to get feedback from someone from the DOM team, anyway.
|
|
Assignee | |
Comment 8•6 years ago
|
||
|
|
Assignee | |
Comment 9•6 years ago
|
||
Depends on D24909
|
||
Comment 10•6 years ago
|
||
|
||
Comment 11•6 years ago
|
||
Backed out 2 changesets (bug 1538968) for causing xpcshell failures on components/places/History.cpp CLOSED TREE
Backout revision https://hg.mozilla.org/integration/autoland/rev/2e6a1dcc4205f8e27b6ae68e057da533b3f0d22a
Failure log https://treeherder.mozilla.org/logviewer.html#?job_id=236531194&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=236531225&repo=autoland
:Gijs caould you please take a look?
|
|
Assignee | |
Updated•6 years ago
|
|
||
Comment 12•6 years ago
|
||
Comment on attachment 9053631 [details]
Bug 1538968 - release assert if something tries to start the history service before profile startup, r?mak
Revision D24910 was moved to bug 1540170. Setting attachment 9053631 [details] to obsolete.
|
|
||
Comment 13•6 years ago
|
||
|
|
Assignee | |
Updated•6 years ago
|
|
||
Comment 14•6 years ago
|
||
| bugherder | ||
Description
•