Closed Bug 1382923 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attachment #8888621 - Flags: review?(michael)
Assignee: nobody → ehsan
Attachment #8888621 - Flags: review?(michael) → review+
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b2ab38ba0b8
Avoid AddRef()ing the History service in the Link constructor; r=mystor
Backout by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/879b737111d9
Backout because History.h doesn't exist on Andoid
Bug 1382920 will give us another option for fixing this.
Depends on: 1382920
(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.
Flags: needinfo?(bobbyholley)
(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?
Flags: needinfo?(bobbyholley) → needinfo?(jryans)
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?
(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
Flags: needinfo?(jryans)
FWIW, I think that my observations in bug 1378275 were just artifacts of other visited-related bugs in stylo.
(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.
Attachment #8888621 - Attachment is obsolete: true
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
Attachment #8901391 - Flags: review?(bugs) → review+
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e74a3b43160
Avoid AddRef()ing the History service in the Link constructor; r=smaug
https://hg.mozilla.org/mozilla-central/rev/4e74a3b43160
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: 1412647
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.