Closed Bug 568969 Opened 9 years ago Closed 9 years ago

Nuke nsContentUtils::GetHistory

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b9
Tracking Status
blocking2.0 --- final+

People

(Reporter: stechz, Assigned: mak)

References

Details

(Whiteboard: [fixed-in-places])

Attachments

(1 file, 2 obsolete files)

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.
Assignee: nobody → dougt
Status: NEW → ASSIGNED
Depends on: 556400
Summary: Nuke nsContentUtils → Nuke nsContentUtils::GetHistory
I do not understand what needs to get done here.  Do we really want to remove: nsContentUtils::GetHistory?
(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)
(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
Oops, I meant to link to bug 556400.  The global services is already done in the original bug.
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?
previous comment was done on the wrong bug...
Attached patch v1.0 (obsolete) — Splinter Review
Note that "fixed pronto" should not equate to no patch after two months.

I think this fixes bug 549887.
Assignee: doug.turner → sdwilsh
Attachment #461024 - Flags: review?(bzbarsky)
Whiteboard: [needs review bz]
Attached patch v1.0 (obsolete) — Splinter Review
Attachment #461024 - Attachment is obsolete: true
Attachment #461024 - Flags: review?(bzbarsky)
Also, sucky that this means we AddRef and Release a bunch now..
Attachment #461026 - Flags: review?(bzbarsky)
Attachment #461026 - Flags: review?(bzbarsky) → review+
Whiteboard: [needs review bz] → [needs approval]
Attachment #461026 - Flags: approval2.0?
pronto, yeah probably not... but we are still working on finish up the e10s work on places.  and of course, lots of other stuff.
> 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.
Attachment #461026 - Flags: approval2.0? → approval2.0+
Whiteboard: [needs approval] → [can land]
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).
Duplicate of this bug: 549887
Carrying over the blocking flag from now duplicate bug 549887.
blocking2.0: --- → final+
http://hg.mozilla.org/mozilla-central/rev/e6e0200b6e03
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [can land]
Target Milestone: --- → mozilla2.0b4
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Shawn - this was backed out for orangeness 2 months ago - does it still need to block?
(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.
(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?
(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
The blocking aspect is that trying to start with no profile will fail layout module init, no?
Or at least if places init fails layout module init will also fail.
(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.
(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.
(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.
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.
Blocks: 606157
(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...
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.
tentatively stealing this.
Assignee: sdwilsh → mak77
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".
Whiteboard: [needs feedback bzbarsky or dbaron on comment 34]
(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.
Attached patch patch v1.1Splinter Review
targeted requests are easier to track!
Attachment #461026 - Attachment is obsolete: true
Attachment #492060 - Flags: review?
Attachment #492060 - Flags: review? → review?(bzbarsky)
Comment on attachment 492060 [details] [diff] [review]
patch v1.1

r=me, though the extra member is sadmaking.
Attachment #492060 - Flags: review?(bzbarsky) → review+
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.
Whiteboard: [needs feedback bzbarsky or dbaron on comment 34]
http://hg.mozilla.org/projects/places/rev/9d7cc07690d2
Status: REOPENED → ASSIGNED
Whiteboard: [fixed-in-places]
Blocks: 585594
OS: Linux → All
Hardware: x86 → All
Target Milestone: mozilla2.0b4 → ---
http://hg.mozilla.org/mozilla-central/rev/9d7cc07690d2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b9
You need to log in before you can comment on or make changes to this bug.