Crash when closing window while tooltip is about the be shown or hidden

VERIFIED FIXED

Status

()

Core
Layout
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

({crash, verified1.8.0.7, verified1.8.1})

Trunk
x86
Linux
crash, verified1.8.0.7, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 +
blocking1.8.0.7 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(7 attachments, 3 obsolete attachments)

(Assignee)

Description

11 years ago
testcase and possible patch coming.
Marking security sensitive, since this is about accessing deleted objects.
(Assignee)

Comment 1

11 years ago
Created attachment 227944 [details]
testcase
(Assignee)

Comment 2

11 years ago
Created attachment 227945 [details] [diff] [review]
'minimal' patch to fix crashes in testcase
(Assignee)

Comment 3

11 years ago
Comment on attachment 227945 [details] [diff] [review]
'minimal' patch to fix crashes in testcase

Wrong patch.
Attachment #227945 - Attachment is obsolete: true
(Assignee)

Comment 4

11 years ago
Created attachment 227948 [details]
testcase (the file for iframe)
Attachment #227944 - Attachment is obsolete: true
(Assignee)

Comment 5

11 years ago
Created attachment 227950 [details]
The testcase
(Assignee)

Comment 6

11 years ago
Created attachment 227951 [details] [diff] [review]
possible patch

Is this too ugly? There are probably similar problems also elsewhere in layout/ 
and using nsFrameDestroyNotifier fixing those should be quite easy.
This shouldn't slow things down too much since mDestroyNotifiers list
is almost always empty and nsVoidArray::Count() is fast operation.
(Assignee)

Comment 7

11 years ago
This is ofc related to roc's safe event dispatching bug, but for 1.8
we need to fix the crashes anyway. (and I may not like the way safe dispatching changes the time when events are dispatched)
(Assignee)

Updated

11 years ago
Attachment #227951 - Flags: review?(roc)
(Assignee)

Comment 8

11 years ago
Fortunately the situation is not that bad.
As far as I see, pretty much all code which should be fixed is in layout/xul

Comment 9

11 years ago
What is the stack signature for this crash?  I'm curious whether it's a topcrash and whether it's the same signature/bug as bug 184363.
(In reply to comment #7)
> This is ofc related to roc's safe event dispatching bug

That's not going anywhere, because I don't really like the approach.
How is this new mechanism different from NS_FRAME_EXTERNAL_REFERENCE?
Also, this is related to bug 279703.
The patch here might make for a better branch fix than the work in bug 279703, if we can actually fix all the important bugs without the redesign in bug 279703.

A way to go here might be to reimplement nsFrameNotifier using NS_FRAME_EXTERNAL_REFERENCE.
(Assignee)

Comment 14

11 years ago
(In reply to comment #9)
> What is the stack signature for this crash?  I'm curious whether it's a
> topcrash and whether it's the same signature/bug as bug 184363.

This is not the same bug as 184363.
(I was actually trying to fix that, but couldn't reproduce it. And while reading the code it came quite clear that popups could crash.)


(Assignee)

Comment 15

11 years ago
I'd actually remove NS_FRAME_EXTERNAL_REFERENCE and just use 
nsFrameDestroyNotifier also in ESM (although no need for that right now). 
The problem here is that if notifier marks 
frame with NS_FRAME_EXTERNAL_REFERENCE bit, and then before notifier is deleted 
ESM wants to mark frame also to have NS_FRAME_EXTERNAL_REFERENCE, then when 
notifier is deleted the bit would be removed although ESM thinks that it is still there.
nsFrameDestroyNotifier could just not remove the EXTERNAL bit.

But yeah, this could be a better way. If we restrict nsFrameDestroyNotifier to be used only on the stack of the main thread, then you don't need an array in the prescontext. We can make nsFrameDestroyNotifiers form a singly linked list.
(Assignee)

Comment 17

11 years ago
Created attachment 228035 [details] [diff] [review]
using single linked list

Using a single linked list, but supporting notifiers in heap.
Though heap usage is not in anyway optimized - if ESM starts using notifiers
easy optimization would be to start marking frames which have notifier with 
NS_FRAME_EXTERNAL_REFERENCE. (In that case I'd rename the macro, perhaps NS_FRAME_MAY_HAVE_NOTIFIER)

Currently when all notifiers are in stack this should be fast, since mDestroyNotifiers is pretty much always nsnull.
Assignee: nobody → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #228035 - Flags: review?(roc)
(Assignee)

Updated

11 years ago
Attachment #227951 - Flags: review?(roc)
Comment on attachment 228035 [details] [diff] [review]
using single linked list

nsFrameDestroyNotifier needs a comment explaining what it's for and how it's meant to be used.

+  nsFrameDestroyNotifier(nsIFrame* aFrame)
+  {
+    mPrev = nsnull;
+    mShell = aFrame ? aFrame->GetPresContext()->GetPresShell() : nsnull;
+    mFrame = mShell ? aFrame : nsnull;
+    if (mShell) {
+      mShell->AddDestroyNotifier(this);
+    }
+  }

Just assert that aFrame is non-null on entry and then go ahead and use it.

Also I don't think you should keep mShell around. Reacquire it in Clear().

I would really like to see ESM start using this itself along with the state bit optimization. Having two separate mechanisms is a recipe for confusion.

+  nsFrameDestroyNotifier popupFrameNotifier(entry->mPopupFrame);
+  NS_ENSURE_STATE(popupFrameNotifier.IsAlive());

move the NS_ENSURE_STATE above and test entry->mPopupFrame.

+  if (notifier.IsAlive()) {
+    // Now open the popup.
+    OpenPopup(entry, PR_TRUE);
+    if (notifier.IsAlive()) {
+      // Now fire the popupshown event.
+      OnCreated(aXPos, aYPos, aPopupContent);
+    }
+  }

Rewrite as "if (!...) return NS_OK;"
Attachment #228035 - Flags: superreview+
Attachment #228035 - Flags: review?(roc)
Attachment #228035 - Flags: review+
(Assignee)

Updated

11 years ago
Blocks: 343613
(Assignee)

Comment 19

11 years ago
I was thinking to change the name nsFrameDestroyNotifier to nsWeakFrame.
nsWeakFrame as a member of a class (for example ESM) makes more sense than 
nsFrameDestroyNotifer.

I hope the name nsWeakFrame is ok.

(I may not have time to check in this before Monday)
That's a good name.

I think you should add comments explaining that usage of this class should be kept to a minimum. Generally speaking, we should avoid having to hold frame references by holding content references instead, and/or moving functionality from frames to content.
(Assignee)

Comment 21

11 years ago
(In reply to comment #18)
> 
> Just assert that aFrame is non-null on entry and then go ahead and use it.
> 

Supporting null nsFrame is useful, then there is no need for some ifs in popupsetframe for example.
(Assignee)

Comment 22

11 years ago
Created attachment 228803 [details] [diff] [review]
This was checked in

Using the name "nsWeakFrame", no mShell member, leaving ability to pass
null nsIFrames to nsWeakFrame constructor.
(Assignee)

Comment 23

11 years ago
This may have affected btek tp. Going to back out.
(Assignee)

Comment 24

11 years ago
Is it ok if I retry to check in nsWeakFrame patch with the change that in nsIPresShell::AddWeakFrame I add aWeakFrame->GetFrame()->AddStateBits(NS_FRAME_EXTERNAL_REFERENCE);
and move the changes from ::NotifyDestroyingFrame to ::ClearFrameRefs.
This way weak references are cleared only when nsFrame has the external bit set.
This is actually pretty much what comment #13 suggests.

I'm not sure the patch caused much of the tp regression.
The noise on btek has been quite bad :(
Yes, go for it.
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Fwiw, at least some of the code being touched here should simply reget the frame after doing the DOM stuff it does, instead of just bailing out if that particular frame has gotten destroyed.  At least imo.
Yeah, I was pushing that point in another bug...
(Assignee)

Updated

11 years ago
Flags: blocking1.8.1?
(Assignee)

Comment 28

11 years ago
I think this should go into 1.8 too.
If agreed, I'll make a branch version of the patch.

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+
(Assignee)

Comment 29

11 years ago
Created attachment 231081 [details] [diff] [review]
for 1.8.1

Roc, could you review this one too. The changes to nsPopupSetFrame.* and nsPopupBoxObject.* are a bit different comparing to trunk patch,
especially methods nsPopupBoxObject::HidePopup and nsPopupBoxObject::ShowPopup.
Attachment #231081 - Flags: superreview?(roc)
Attachment #231081 - Flags: review?(roc)
(Assignee)

Comment 30

11 years ago
btw, what is the policy regarding interfaces in layout/ ?
This changes nsIPresShell. Should I create nsIPresShell_MOZILLA_1_8_BRANCH ?
nsIFrame has been changed...

I'll create a new patch with nsIPresShell_MOZILLA_1_8_BRANCH.
(Assignee)

Comment 31

11 years ago
Created attachment 231090 [details] [diff] [review]
using nsIPresShell_MOZILLA_1_8_BRANCH
Attachment #231081 - Attachment is obsolete: true
Attachment #231090 - Flags: superreview?(roc)
Attachment #231090 - Flags: review?(roc)
Attachment #231081 - Flags: superreview?(roc)
Attachment #231081 - Flags: review?(roc)
Attachment #231090 - Flags: superreview?(roc)
Attachment #231090 - Flags: superreview+
Attachment #231090 - Flags: review?(roc)
Attachment #231090 - Flags: review+
> btw, what is the policy regarding interfaces in layout/ ?

Same as elsewhere.

> Should I create nsIPresShell_MOZILLA_1_8_BRANCH ?

Yes.

> nsIFrame has been changed...

AAARGH.  Not just that, but the IID did not change.  :(  That should NOT have happened.  :(
(Assignee)

Updated

11 years ago
Attachment #231090 - Flags: approval1.8.1?

Comment 33

11 years ago
Comment on attachment 231090 [details] [diff] [review]
using nsIPresShell_MOZILLA_1_8_BRANCH

approved by schrep for drivers
Attachment #231090 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.1
Flags: blocking1.8.0.7?
Whiteboard: [sg:critical]
(Assignee)

Updated

11 years ago
Blocks: 346054
Flags: blocking1.8.0.7? → blocking1.8.0.7+
(Assignee)

Comment 34

11 years ago
Created attachment 233055 [details] [diff] [review]
for 1.8.0
(Assignee)

Comment 35

11 years ago
Comment on attachment 233055 [details] [diff] [review]
for 1.8.0

This is basically the same patch as for 1.8, just one line change, the one with FireChromeDOMEvent.
This should go in with the patch in Bug 346054.
Attachment #233055 - Flags: approval1.8.0.7?
Comment on attachment 233055 [details] [diff] [review]
for 1.8.0

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #233055 - Flags: approval1.8.0.7? → approval1.8.0.7+
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.0.7
https://bugzilla.mozilla.org/attachment.cgi?id=227950&action=view should not crash in either tooltip scenario

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7pre) Gecko/20060821 Firefox/1.5.0.7pre

verified 1.8.0.7

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/2006082203 BonEcho/2.0b2

verified 1.8.1b2
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.7, fixed1.8.1 → verified1.8.0.7, verified1.8.1
Keywords: crash
Group: security
You need to log in before you can comment on or make changes to this bug.