Closed Bug 549882 Opened 10 years ago Closed 10 years ago

###!!! ASSERTION: Trying to unregister a node that wasn't registered!

Categories

(Toolkit :: Places, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.9.3a3

People

(Reporter: smaug, Assigned: sdwilsh)

References

Details

Attachments

(1 file, 1 obsolete file)

It seems that pretty much every link generates the following warning/assertion in
E10s when using test.xul.
Links are also missing the default blue color. They look just like normal text.

WARNING: NS_ENSURE_TRUE(navHist) failed: file /home/smaug/mozilla/mozilla_cvs/hg/e10s/toolkit/components/places/src/History.cpp, line 75
###!!! ASSERTION: Trying to unregister a node that wasn't registered!: 'Error', file /home/smaug/mozilla/mozilla_cvs/hg/e10s/toolkit/components/places/src/History.cpp, line 290
Which process is this happening in?
I assume in content process, since chrome process has only a simple xul file
(test.xul)
I assume this would go away with bug 516728 which makes this not run in the content process.  I don't think we want any history-type code running in the content process long term.
These warnings and assertions are now so noisy that they pretty much prevent
reading any useful messages from console.
And also, why is the default color not applied to links?
I don't know what is broken in e10s builds that would cause link coloring not to work.  I also don't know what is broken in e10s builds to cause all the assertions.  However, I'm not going to accept a patch to disable them in mozilla-central because 1) they aren't firing there and 2) they've already found one real bug (bug 546900).  The assertions check invariants that that code assumes to be true at all time, and when it's not true, things can easily break.

The link coloring code is handled in mozilla::dom::Link::LinkState (in Link.cpp).
Hmm.  So that first assert (whatever the reason for it; looks like no navhistory in content process?) would cause Start() to fail.  That would them make RegisterVisitedCallback() unregister (which will trigger the second assert, since it's not in fact registered yet) and fail.

That seems like a bug in mozilla::places::History.  It should do something saner if Start() fails, so as to avoid that second assertion.

Not sure whether we should be warning on failing to get navHist.  It makes it easy to tell what's going on with this bug, but will be really noisy in the meantime. 

> And also, why is the default color not applied to links?

Because if RegisterVisitedCallback fails, at http://hg.mozilla.org/mozilla-central/file/063f18ccd02f/content/base/src/Link.cpp#l132 then we leave mLinkState as unknown.  Perhaps we should set it to unvisited in that case, even though that means we'll never end up with visited styling in the future...
(In reply to comment #6)
> Because if RegisterVisitedCallback fails, at
> http://hg.mozilla.org/mozilla-central/file/063f18ccd02f/content/base/src/Link.cpp#l132
> then we leave mLinkState as unknown.  Perhaps we should set it to unvisited in
> that case, even though that means we'll never end up with visited styling in
> the future...
I'm not handling this in this patch.  I'm not convinced we should do it yet.  I've taken care of the faulty logic though in History.cpp.
Attached patch v1.0 (obsolete) — Splinter Review
When I was using a different hashtable in an earlier implementation, this logic was different and I needed to Unregister at that time.  Do not need to anymore with nsTHashtable, so we can just return.
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Attachment #430119 - Flags: review?(dietrich)
Whiteboard: [needs review dietrich]
Version: unspecified → Trunk
Won't that leave the observers array in the hashtable forever?
(In reply to comment #9)
> Won't that leave the observers array in the hashtable forever?
Er, yes.  I can't write a test for this, but I'm going to add some debug-only code to make sure we catch this issue in the future.
Attached patch v1.1Splinter Review
Now with less leaks and more sanity checking.
Attachment #430119 - Attachment is obsolete: true
Attachment #430124 - Flags: review?(dietrich)
Attachment #430119 - Flags: review?(dietrich)
Comment on attachment 430124 [details] [diff] [review]
v1.1

r=me w/ changes voiced IRL.
Attachment #430124 - Flags: review?(dietrich) → review+
Whiteboard: [needs review dietrich] → [can land]
http://hg.mozilla.org/mozilla-central/rev/37af1321c320
Flags: in-testsuite-
Flags: in-litmus-
OS: Linux → All
Hardware: x86 → All
Whiteboard: [can land]
Target Milestone: --- → mozilla1.9.3a3
Forgot to mark this as fixed...
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.