179089 - crash in selection stuff loading a newsgroup message [@JS_GetPrivate]

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[timeless]

I was reading news.openwatcom.org/openwatcom.contributors
i use n/space to advance messages and i also double click in the message body
sometimes for fear that focus is in the wrong place. the response time for this
session was bad, i'm not sure if that contributed or why it would be the case.

Comment 1

[jag (Peter Annema)]

JS_GetPrivate [d:/builds/seamonkey/mozilla/js/src/jsapi.c, line 1928]
nsScriptSecurityManager::GetFunctionObjectPrincipal
[d:/builds/seamonkey/mozilla/caps/src/nsScriptSecurityManager.cpp, line 1808]
nsScriptSecurityManager::GetFramePrincipal
[d:/builds/seamonkey/mozilla/caps/src/nsScriptSecurityManager.cpp, line 1842]
nsScriptSecurityManager::GetPrincipalAndFrame
[d:/builds/seamonkey/mozilla/caps/src/nsScriptSecurityManager.cpp, line 1855]
nsScriptSecurityManager::GetSubjectPrincipal
[d:/builds/seamonkey/mozilla/caps/src/nsScriptSecurityManager.cpp, line 1895]
nsScriptSecurityManager::GetSubjectPrincipal
[d:/builds/seamonkey/mozilla/caps/src/nsScriptSecurityManager.cpp, line 1607]
nsContentUtils::CanCallerAccess
[d:/builds/seamonkey/mozilla/content/base/src/nsContentUtils.cpp, line 531]
nsRange::SetStart [d:/builds/seamonkey/mozilla/content/base/src/nsRange.cpp,
line 1030]
nsTypedSelection::SetOriginalAnchorPoint
[d:/builds/seamonkey/mozilla/content/base/src/nsSelection.cpp, line 6614]
nsTypedSelection::Collapse
[d:/builds/seamonkey/mozilla/content/base/src/nsSelection.cpp, line 6151]
nsSelection::TakeFocus
[d:/builds/seamonkey/mozilla/content/base/src/nsSelection.cpp, line 2853]
nsSelection::HandleClick
[d:/builds/seamonkey/mozilla/content/base/src/nsSelection.cpp, line 2643]
nsFrame::HandlePress
[d:/builds/seamonkey/mozilla/layout/html/base/src/nsFrame.cpp, line 1499]
nsFrame::HandleEvent
[d:/builds/seamonkey/mozilla/layout/html/base/src/nsFrame.cpp, line 981]
PresShell::HandleEventInternal
[d:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6263]
PresShell::HandleEvent
[d:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6169]
nsViewManager::HandleEvent
[d:/builds/seamonkey/mozilla/view/src/nsViewManager.cpp, line 2209]
nsView::HandleEvent [d:/builds/seamonkey/mozilla/view/src/nsView.cpp, line 304]
nsViewManager::DispatchEvent
[d:/builds/seamonkey/mozilla/view/src/nsViewManager.cpp, line 1949]
HandleEvent [d:/builds/seamonkey/mozilla/view/src/nsView.cpp, line 83]
nsWindow::DispatchEvent
[d:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 1074]
nsWindow::DispatchWindowEvent
[d:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 1091]
nsWindow::DispatchMouseEvent
[d:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 5208]
nsWindow::DispatchFocus
[d:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 5462]
nsWindow::ProcessMessage
[d:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 4060]
nsWindow::WindowProc
[d:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 1339]
USER32.DLL + 0x3eb0 (0x77e13eb0)
USER32.DLL + 0x401a (0x77e1401a)
USER32.DLL + 0x92da (0x77e192da)
nsAppShellService::Run
[d:/builds/seamonkey/mozilla/xpfe/appshell/src/nsAppShellService.cpp, line 472]
main1 [d:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1538]
main [d:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1886]
WinMain [d:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1906]
WinMainCRTStartup()
KERNEL32.DLL + 0x7903 (0x77e87903) 

Comment 2

[timeless]

1927 brendan    3.114     v = GC_AWARE_GET_SLOT(cx, obj, JSSLOT_PRIVATE);
(cc for help and verification of analysis, not blame)

I'm guessing the crash is a destroyed obj.

nsScriptSecurityManager::GetFramePrincipal
appears to verify that obj isn't null

wow, no mailnews code at all. well here's what i think happened:

mozilla was slowed down (and handling events slowly).
so a mouse double click event fired by the old mailnews message was processed
after the message was replaced.

Comment 3

[Scott MacGregor]

I think we should move this bug out from under browser / selection. I don't
think it has to to with selection.

You can reproduce this in the latest thunderbird builds very easily. 

1) bring up mail compose
2) Attach a web page, type in a  url
3) hit the send button
4) we crash with this same stack trace.

Although I just noticed this bug was reported back in 2002. Maybe thunderbird
just introduced a new scenario that leads back to this same crash.

Comment 4

[David Bradley]

Attachment: Fixes the null pointer and consolidates the code

bz apparently fixed this problem for the insert case, but the remove case
wasn't addressed.

The code is nearly identical for the two functions so I moved the common code
to a separate local function to help prevent future maintenance headaches.

Comment 5

[David Bradley]

Oh, I guess some explanation would be good too.

The NS_RELEASE was causing problems because the pointer can be null, as
indicated by surrounding code. This referencing of a null pointer was getting
caught by Win32's structured exception handling. Fixing this, I couldn't
reproduce There's two possibilities from here.

1. The caught exception left objects in an odd state and this was the source of
the faults in JS
2. Correcting this problem effectively hides what's causing these crashes

There's still a lot of asserts firing, but I was getting those before.

Comment 6

[Scott MacGregor]

That's great news David. Thanks a lot for the quick turn around on this. I'll
try your patch out in thunderbird today and verify that it also works for me. 

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 127462 [details] [diff] [review]
Fixes the null pointer and consolidates the code

- In NotifyListBoxBody(nsIPresContext*	  aPresContext,
+		       nsIContent*	  aContainer,
+		       nsIContent*	  aChild,
+		       PRInt32		  aIndexInContainer,
+		       nsIDocument*	  aDocument,			     
+		       nsIFrame*	  aChildFrame,
+		       PRBool		  useXBLForms,
+		       content_operation  operation)

Maybe rename useXBLForms to aUseXBLForms and operation to aOperation to be
consistent with the other arguments?

+{
+  if (aContainer) {
...
   }
+  return PR_FALSE;

How about:

+{
+  if (!aContainer) {
+    return PR_FALSE;
+  }
+  ...

to avoid indenting the whole function?

+    PRBool listitem = tag && tag.get() == nsXULAtoms::listitem;
+    if (listitem) {

|listitem| isn't used for anything except that if check, maybe loose it, and
change the above to:

+    if (tag == nsXULAtoms::listitem;

(no need for the .get(), and nsXULAtoms::listitem isn't null unless something
went really wrong during startup, in which case we should never get here).


- In nsCSSFrameConstructor::ContentInserted():

+  if (aContainer && NotifyListBoxBody(aPresContext, aContainer, aChild, 
+				       aIndexInContainer, mDocument, nsnull,
+				       UseXBLForms(), CONTENT_INSERTED))

NotifyListBoxBody returns false if (!aContainer), so no need to check it here
before making the call.

- In nsCSSFrameConstructor::ContentRemoved():

+  if (aContainer && NotifyListBoxBody(aPresContext, aContainer, aChild, 
+				       aIndexInContainer, mDocument,
childFrame,
+				       UseXBLForms(), CONTENT_REMOVED))

Same thing, no need to check aContainer.

sr=jst

Comment 8

[Brendan Eich [:brendan]]

Who will r=?  Should we get this into 1.5a for some widely-distributed testing?

/be

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 127462 [details] [diff] [review]
Fixes the null pointer and consolidates the code

What jst said, and...

>+static
>+PRBool NotifyListBoxBody(nsIPresContext*    aPresContext,

....

>+      } else {
>+        // There is no frame for the removed/inserted content, so we need to dig
>+        // further for the body frame. XBL insertion may not yet have taken
>+        // place here (in the case of  replacements), so our only connection

Nit: Could you remove the weird double space in between "of" and
"replacements"?

>+        // to the listbox content is through aContainer.  Use the boxObject
>+        // to get to the listboxbody frame so we can notify it of the removal.

This comment applies to both insertion and removal cases.  Please reflect that
in the last line of the comment (it currently pertains to only removal).

Comment 10

[David Bradley]

Should I seek approval for 1.5a? Given that this is blocking a feature for
Thunderbird and might be responsible for some other similiar screwy crashes it
seems like a decent win. I also noticed about a crash a day listed for the
ContentRemoved method as well.

Comment 11

[David Bradley]

Patch checked in