1382923 - Avoid AddRef()ing the History service in the Link constructor

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Comment 1

[(no longer active)]

Attachment: Avoid AddRef()ing the History service in the Link constructor

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b2ab38ba0b8
Avoid AddRef()ing the History service in the Link constructor; r=mystor

Comment 3

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/879b737111d9
Backout because History.h doesn't exist on Andoid

Comment 4

[(no longer active)]

Bug 1382920 will give us another option for fixing this.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This also caused crashes for Marionette tests:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=mn&selectedJob=116433978

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

And debug stylo reftests: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=abd1b9db490509a1d029ab438fe8714d28328fe4&noautoclassify&filter-searchStr=stylo%20ref&group_state=expanded&tochange=879b737111d925ea97eb1caabf339bdc6fbca36f

Comment 7

[(no longer active)]

(In reply to Wes Kocher (:KWierso) from comment #6)
> And debug stylo reftests:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&fromchange=abd1b9db490509a1d029ab438fe8714d28328fe4&noautoclassify&fi
> lter-
> searchStr=stylo%20ref&group_state=expanded&tochange=879b737111d925ea97eb1caab
> f339bdc6fbca36f

Oh, hmm, so these show that this service is actually needed pretty late during shutdown, and for some reason only in a stylo build.  There is an assertion that we hit before these which is also interesting:

ASSERTION: Not all Links were removed before we disappear!: 'mObservers.Count() == 0', file /home/worker/workspace/build/src/toolkit/components/places/History.cpp, line 1946

This is from the destructor of History.  It seems like on stylo builds, these objects actually need to hold the history service alive!

Bobby, do you know by any chance why there is a difference between Stylo and non-Stylo builds in the timing of the destruction of Link objects?  This is pretty surprising to me.

Comment 8

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #7)
> (In reply to Wes Kocher (:KWierso) from comment #6)
> Bobby, do you know by any chance why there is a difference between Stylo and
> non-Stylo builds in the timing of the destruction of Link objects?  This is
> pretty surprising to me.

I don't think so. Link objects relate to the style system in terms of visited handling, but I don't _think_ we store a strong reference to Link objects anywhere. jryans?

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

So the actual crash is
> Assertion failure: service (Cannot obtain IHistory service!)

And from the stack of that, it seems to be from element unbinding from cycle collection. So the Links are actually hold by the elements, and the elements may be somehow held in Stylo for longer than Gecko?

What guarantees that the elements which hold Links would go away before we shutting down History in Gecko?

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #8)
> (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from
> comment #7)
> > (In reply to Wes Kocher (:KWierso) from comment #6)
> > Bobby, do you know by any chance why there is a difference between Stylo and
> > non-Stylo builds in the timing of the destruction of Link objects?  This is
> > pretty surprising to me.
> 
> I don't think so. Link objects relate to the style system in terms of
> visited handling, but I don't _think_ we store a strong reference to Link
> objects anywhere. jryans?

AFAIK, we aren't holding on to Link objects explicitly with Stylo, but there is an unresolved behavior difference in the timing of link processing between Gecko and Stylo.  Specifically:

* The method `nsIDocument::FlushPendingLinkUpdates` will register links with the history service (via `Link::LinkState`)
* Gecko calls `FlushPendingLinkUpdates` during restyles[1] (among other places), but :bz and I seemed to think a few weeks ago that it would be safe to remove these.  Bug 1361618 tracks that removal.
* Stylo does not currently call `FlushPendingLinkUpdates` during restyles, but :emilio has argued in bug 1378275 that potentially we should go the opposite direction and call it like Gecko currently does.

I was planning to take another look at this area soon, and so far I've been leaning towards landing the removal of Gecko's call during restyle (bug 1361618), unless we discover new evidence that it's actually needed.

Anyway, that's the most likely reason I can think of for a timing difference in link processing for Gecko vs. Stylo at the moment.

[1]: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/layout/base/GeckoRestyleManager.cpp#1748

Comment 11

[Emilio Cobos Álvarez (:emilio)]

FWIW, I think that my observations in bug 1378275 were just artifacts of other visited-related bugs in stylo.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #9)
> And from the stack of that, it seems to be from element unbinding from cycle
> collection. So the Links are actually hold by the elements, and the elements
> may be somehow held in Stylo for longer than Gecko?
Elements are Links
http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/dom/html/HTMLAnchorElement.h#21,23

 
> What guarantees that the elements which hold Links would go away before we
> shutting down History in Gecko?
Nothing guarantees Elements are gone before History.
As far as I see that patch is just buggy assuming History::GetService() always returns non-null and that something keeps the service alive long enough so that unregister can be called even during shutdown.

But it is probably ok to change History so that History::~History() doesn't assert and Link should null check before accessing History::GetService()'s return value.

Comment 13

[(no longer active)]

Attachment: Avoid AddRef()ing the History service in the Link constructor

Comment 14

[(no longer active)]

I removed the assertion on mCachedURI inside UnregisterFromHistory() because there isn't anything that guarantees it to be non-null any more, and moved the call to this function in ResetLinkState() as a best effort attempt to do this before setting mCachedURI to null.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d4d56924330aa64812593ccafcf35f83aa71805

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e74a3b43160
Avoid AddRef()ing the History service in the Link constructor; r=smaug

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4e74a3b43160