Closed Bug 351347 Opened 13 years ago Closed 12 years ago

Crash at [@ nsXULDocument::ExecuteOnBroadcastHandlerFor][@ 0x02a1ff9a] [@ nsCOMPtr<nsIContent>::nsCOMPtr<nsIContent>]

Categories

(Core :: XUL, defect, critical)

x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED DUPLICATE of bug 386914

People

(Reporter: rstrong, Assigned: enndeakin)

References

Details

(Keywords: crash)

Crash Data

Spinoff from bug 345006 comment #42

When we crash, is it because in nsXULDocument::ExecuteOnBroadcastHandlerFor()
we are passing in garbage to do_QueryInterface()?  

Looking at the stack, we call nsXULDocument::ExecuteOnBroadcastHandlerFor()
from nsXULDocument::AttributeChanged(), and in
nsXULDocument::AttributeChanged() we do the same do_QueryInterface() on the
same nsIDOMElement pointer.  But before we call
nsXULDocument::ExecuteOnBroadcastHandlerFor(), we call listener->SetAttr() or
listener->UnsetAttr().  

Could one of those methods have a side effect of releasing the nsIDOMElement
that bl (or bl->mListener) was pointing to?

From nsXULDocument.cpp:

348 struct BroadcastListener {
349     nsIDOMElement*    mListener; // [WEAK] XXXwaterson crash waiting to
happen!
350     nsCOMPtr<nsIAtom> mAttribute;
351 };
*** Bug 351348 has been marked as a duplicate of this bug. ***
thanks for logging this, robert.

is this a Core / DOM bug?
Can someone point me to steps to reproduce?  I can do a little work on this.
From Bug 351348 - this was seen when calling "ensureElementIsVisible on an element that no longer exists" for an element in a richlistbox.
additional, if you have a build without robert's fix for bug #345006: see https://bugzilla.mozilla.org/show_bug.cgi?id=345006#c31 for steps.

but note robert's comments #33, #35, and #36 in that bug, as there might be a trick to reproducing it.


Severity: normal → critical
> Could one of those methods have a side effect of releasing the nsIDOMElement
> that bl (or bl->mListener) was pointing to?

Yes.  In fact, it could have a side effect of making |bl| point to deleted memory.  It should be pretty easy to write testcases that do that.

In my opinion we should hold a stack nsCOMPtr to bl->mListener in nsXULDocument::AttributeChanged.  Or something.  What _should_ happen if the broadcast listener is removed during that SetAttr()?
Flags: blocking1.9?
We shouldn't be calling SetAttr during AttributeChanged. We should wait with calling that until the last EndUpdate call is done.
Sure, that would work too.
Anyone has any feel for if broadcasting attribute changes later could break stuff? Neil?
(In reply to comment #9)
>Anyone has any feel for if broadcasting attribute changes later could break
>stuff?
What happens if someone uses a mutation listener to "fix" incorrect attributes?
Then the attribute would change to the 'wrong' value first (and we'd react to it), and then change back to the correct value.
Just as long as we would't get duplicate or misordered events then [I'm thinking here of the focus() in anchor focus handler confuses status bar bug].
Could you tell me exactly what needs to happen there? I.e. what attribute gets set and reset to what and which events need to fire.

There is a risk that we'll get duplicate events, but they shouldn't be missordered. In fact, with the proposed changes missordering should be less likely than now.
(In reply to comment #13)
>with the proposed changes missordering should be less likely than now.
That sounds great.
Keywords: crash
Flags: blocking1.9? → blocking1.9+
Neil, can you please take this?
Assignee: nobody → enndeakin
Depends on: 386914
Note that we also call SetAttr during AttributeChanged for xbl:inherits stuff, so maybe it's not something to really worry about...
qawanted: the fix for bug 386914 has landed now, did these turn out to be the same thing after all? (Can this be closed fixed or dupe?)
Keywords: qawanted
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007080204 Minefield/3.0a7pre

I think this is a dupe, yes. In bug 345006, this was essentially wallpapered over by the Extension Manager, and rob_strong filed this bug for the underlying issue. 

Just for good measure, I tried reproducing using both the steps in bug 345006, comment 31 and Martijn's testcase in bug 386914 and was unsuccessful.

FWIW, there are only two crashes in Breakpad for this, one of which is Martijn's (in bug 386914).
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: qawanted
Resolution: --- → DUPLICATE
Duplicate of bug: 386914
Flags: blocking1.9+
Flags: blocking1.9+
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
Crash Signature: [@ nsXULDocument::ExecuteOnBroadcastHandlerFor] [@ 0x02a1ff9a] [@ nsCOMPtr<nsIContent>::nsCOMPtr<nsIContent>]
You need to log in before you can comment on or make changes to this bug.