Benjamin Stover (:stechz) Reporter
	Description • 14 years ago

See https://bugzilla.mozilla.org/show_bug.cgi?id=556400#c104

	Shawn Wilsher :sdwilsh
	Comment 1 • 14 years ago

I have a promise from the mobile team that this stuff would get fixed pronto if I let them do it in a follow-up bug.

	Doug Turner (:dougt)
	Comment 2 • 14 years ago

I do not understand what needs to get done here.  Do we really want to remove: nsContentUtils::GetHistory?

	Shawn Wilsher :sdwilsh
	Comment 3 • 14 years ago

(In reply to comment #2)
> I do not understand what needs to get done here.  Do we really want to remove:
> nsContentUtils::GetHistory?
Plus add to the global services (which may be done in a different bug?  I can't recall)

	Benjamin Stover (:stechz) Reporter
	Comment 4 • 14 years ago

(In reply to comment #3)
> (In reply to comment #2)
> > I do not understand what needs to get done here.  Do we really want to remove:
> > nsContentUtils::GetHistory?
> Plus add to the global services (which may be done in a different bug?  I can't
> recall)

https://bugzilla.mozilla.org/show_bug.cgi?id=568971

	Benjamin Stover (:stechz) Reporter
	Comment 5 • 14 years ago

Oops, I meant to link to bug 556400.  The global services is already done in the original bug.

	Shawn Wilsher :sdwilsh
	Comment 6 • 14 years ago

So, as I understand it, IHistory is being created during startup of the content process.  Although that, in itself, shouldn't be a problem based on code inspection.

Did this magically get fixed?

	Shawn Wilsher :sdwilsh
	Comment 7 • 14 years ago

previous comment was done on the wrong bug...

	Shawn Wilsher :sdwilsh
	Comment 8 • 14 years ago

Note that "fixed pronto" should not equate to no patch after two months.

I think this fixes bug 549887.

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 14 years ago

Comment on attachment 461024 [details] [diff] [review]
v1.0

You forgot to qref?

	Shawn Wilsher :sdwilsh
	Comment 11 • 14 years ago

Also, sucky that this means we AddRef and Release a bunch now..

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 14 years ago

Comment on attachment 461026 [details] [diff] [review]
v1.0

r=bzbarsky

	Doug Turner (:dougt)
	Comment 13 • 14 years ago

pronto, yeah probably not... but we are still working on finish up the e10s work on places.  and of course, lots of other stuff.

	Benjamin Stover (:stechz) Reporter
	Comment 14 • 14 years ago

> Note that "fixed pronto" should not equate to no patch after two months.

Shawn: the patch this depended on just landed less than two weeks ago, and this is merely a cleanup bug.  We are working on shipping a product, and I'm sure you understand how this slipped through the cracks.

Give us some slack.  This is ridiculous.

	Shawn Wilsher :sdwilsh
	Comment 15 • 14 years ago

Somehow I missed ContentParent.cpp in my patch.  It's three lines, so I just fixed it locally (I don't think anybody cares to see it).

	Shawn Wilsher :sdwilsh
	Comment 17 • 14 years ago

Carrying over the blocking flag from now duplicate bug 549887.

	Shawn Wilsher :sdwilsh
	Comment 18 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/e6e0200b6e03

	Shawn Wilsher :sdwilsh
	Comment 19 • 14 years ago

And backed out because this appears to have caused the world to go orange.  I thought I pushed this to the try server...

http://hg.mozilla.org/mozilla-central/rev/70dd3ffe088b
http://hg.mozilla.org/mozilla-central/rev/4572ba19acb6

	Shawn Wilsher :sdwilsh
	Comment 20 • 14 years ago

Backout commits were actually these:
http://hg.mozilla.org/mozilla-central/rev/e88f6c524f3a
http://hg.mozilla.org/mozilla-central/rev/1e4f621a084d

	Johnathan Nightingale [:johnath]
	Comment 21 • 14 years ago

Shawn - this was backed out for orangeness 2 months ago - does it still need to block?

	Shawn Wilsher :sdwilsh
	Comment 22 • 14 years ago

(In reply to comment #21)
> Shawn - this was backed out for orangeness 2 months ago - does it still need to
> block?
You'd have to ask the folks in bug 549987 (the reason why this bug is blocking) if the bug is still present.  I haven't ever been able to reproduce it or get steps to reproduce, but fixing this does remove all that code so it certainly fixes that issue.

If that bug isn't visible on trunk anymore, then I don't see why this needs to block.

	Johnathan Nightingale [:johnath]
	Comment 23 • 14 years ago

(In reply to comment #22)
> (In reply to comment #21)
> > Shawn - this was backed out for orangeness 2 months ago - does it still need to
> > block?
> You'd have to ask the folks in bug 549987 (the reason why this bug is blocking)
> if the bug is still present.  I haven't ever been able to reproduce it or get
> steps to reproduce, but fixing this does remove all that code so it certainly
> fixes that issue.
> 
> If that bug isn't visible on trunk anymore, then I don't see why this needs to
> block.

Doug, stechz?

	Shawn Wilsher :sdwilsh
	Comment 24 • 14 years ago

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > Shawn - this was backed out for orangeness 2 months ago - does it still need to
> > > block?
> > You'd have to ask the folks in bug 549987 (the reason why this bug is blocking)
> > if the bug is still present.  I haven't ever been able to reproduce it or get
> > steps to reproduce, but fixing this does remove all that code so it certainly
> > fixes that issue.
> > 
> > If that bug isn't visible on trunk anymore, then I don't see why this needs to
> > block.
> 
> Doug, stechz?
And I meant bug 549887, which isn't doug or stechz but rather hsivonen/bz/dbaron

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 14 years ago

The blocking aspect is that trying to start with no profile will fail layout module init, no?

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 14 years ago

Or at least if places init fails layout module init will also fail.

	Shawn Wilsher :sdwilsh
	Comment 27 • 14 years ago

(In reply to comment #26)
> Or at least if places init fails layout module init will also fail.
Yes, but I wasn't able to reproduce it when I last tried, and when I asked in the other bug nobody ever responded if they could still reproduce it.

	Shawn Wilsher :sdwilsh
	Comment 28 • 14 years ago

(In reply to comment #26)
> Or at least if places init fails layout module init will also fail.
Rather, we were seeing this with normal builds as I understood it.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 29 • 14 years ago

(In reply to comment #24)
> And I meant bug 549887, which isn't doug or stechz but rather
> hsivonen/bz/dbaron

I haven't seen that bug occur again in a while.

	Marco Bonardo [:mak] Assignee
	Comment 30 • 14 years ago

FWIW, as bz said this tries to start Places when there is no profile yet, and that is a Ts hit and a bunch of warnings on startup.
I'll try to push this to try and see where we are.

	Shawn Wilsher :sdwilsh
	Comment 31 • 14 years ago

(In reply to comment #30)
> FWIW, as bz said this tries to start Places when there is no profile yet, and
> that is a Ts hit and a bunch of warnings on startup.
> I'll try to push this to try and see where we are.
You shouldn't have to push this to try.  Just apply the patch, compile, and start the browser and I think I hit null derefrences.  xpcshell tests were fine though...

	Marco Bonardo [:mak] Assignee
	Comment 32 • 14 years ago

slightly modified version on try (full green)
http://hg.mozilla.org/try/rev/88915e70be95

I think the null deref is just history inited without a profile or collected before the link.

	Marco Bonardo [:mak] Assignee
	Comment 33 • 14 years ago

tentatively stealing this.

	Marco Bonardo [:mak] Assignee
	Comment 34 • 14 years ago

So, History could die before all links have (thus history asserts that not all links have been unregistered) and links could try to dereference a dead History.
This was not a big deal before since looks like ContentUtils was holding History alive long enough to hide the ownership problem.
There are 2 possible solutions that I'd like to have feedback on from bzbarsky or dbaron (or both!):

1. Make Link hold a strong reference to History.  This is the more correct solution from a registration point of view, History can't disappear till all links have.  I have currently implemented this solution and it's running on tryserver (http://hg.mozilla.org/try/rev/68d064969d83 for reference).  The drawback could be that each link has to addref/release, could this be a perf loss we don't want to pay?

2. Make Link aware that History can disappear at any time and make History aware that not all links could be unregistered when it is destroyed.  This makes things weak pointing each other, requires null checks everywhere the 2 cross, some current checks on correctness of unregistration have to be removed (like the fact all links have been unregistered).

The first solution could be more expensive for addrefing (any idea how much it would hurt?), the second solution involves more attention when code crosses and is less "correct".

	Shawn Wilsher :sdwilsh
	Comment 35 • 14 years ago

(In reply to comment #34)
> 2. Make Link aware that History can disappear at any time and make History
> aware that not all links could be unregistered when it is destroyed.  This
> makes things weak pointing each other, requires null checks everywhere the 2
> cross, some current checks on correctness of unregistration have to be removed
> (like the fact all links have been unregistered).
Another issue here is that it gets to be more difficult to track if we happen to start leaking Link objects.  The assertion at shutdown made sure we never kept them registered.

	Marco Bonardo [:mak] Assignee
	Comment 36 • 14 years ago

targeted requests are easier to track!

	Boris Zbarsky [:bzbarsky]
	Comment 37 • 14 years ago

Comment on attachment 492060 [details] [diff] [review]
patch v1.1

r=me, though the extra member is sadmaking.

	Marco Bonardo [:mak] Assignee
	Comment 38 • 14 years ago

I assume the approval is still valid since the idea at the base of the patch did not change, the changes are mostly a ownership change.
Also, I'll land this on Places branch, to have even more baking.

	Marco Bonardo [:mak] Assignee
	Comment 39 • 14 years ago

http://hg.mozilla.org/projects/places/rev/9d7cc07690d2

	Marco Bonardo [:mak] Assignee
	Comment 40 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/9d7cc07690d2