193847 - crash with "pure virtual method called" on Javascript DOM modification

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[robinson]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030210
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030210

On the page <http://research.salutia.com/tiki/tiki-view_faq.php?faqId=1>,
clicking the "show comments" link at the bottom immediately crashes the browser
with "pure virtual method called", and no talkback window.

Reproducible: Always

Steps to Reproduce:
See details.
Actual Results:  
Immediate crash with "pure virtual method called".

Expected Results:  
Show comments.

Comment 1

[Andrew Schultz]

Attachment: stacktrace

Comment 2

[Andrew Schultz]

==> DOM0 (?)

marking NEW

Comment 3

[Andrew Schultz]

regression between 2003010805 and 2003010905

Comment 4

[Boris Zbarsky [:bzbarsky]]

Actaully, it regressed between 2003010821 and 2003010905

In that range, there's bryner's checkin to deCOMify stylecontext.  The code
that's most likely dying in nsElementSH::PostCreate is:

4575   const nsStyleDisplay* display;
4576   pctx->ResolveStyleContextAndGetStyleData(content, eStyleStruct_Display,
4577                                            (const nsStyleStruct*&) display);
4578   NS_ENSURE_TRUE(display, NS_ERROR_UNEXPECTED);
4579 
4580   if (display->mBinding.IsEmpty()) {

Maybe we're getting garbage out for "display"?

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

ResolveStyleContextAndGetStyleData is broken.  One needs to hold on to the style
context while using the data.  We should probably just remove the method.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Well, then we have a few options:

1)  link all this code in with layout so it can use nsIStyleContext
2)  Go back to using computed style (with the resulting perf hit)
3)  Make the method clearly XBL-specific since this is the only user.  eg, on
    nsIPresContext:

    NS_IMETHOD HasXBLBinding(nsIContent* aContent, PRBool* aHasBinding);

I like #3 myself, as a short-term measure....

Comment 7

[joakim_46]

Confirmed with build 2003021008 under Windows XP SP1.

Talkback ID: TB17296928Z

Comment 8

[Mark Cooper]

*** Bug 194181 has been marked as a duplicate of this bug. ***

Comment 9

[Asa Dotzler [:asa]]

Is this something we're likely to run into a lot in the real world? Should this
block 1.3?

Comment 10

[Boris Zbarsky [:bzbarsky]]

I'm surprised it's not come up more than it has -- we're basically reading
potentially garbage memory on some-to-all element creations.

If bryner has absolutely no time for this, I can try to whip up a patch... (but
I'm really swamped with trying to not fail a class or two right now, so I really
mean _absolutely_ no time).

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I can take it.

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

er, really

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: patch

(I haven't tested this yet, since it's still recompiling.)

Comment 14

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: patch

Correct comment; remove extra whitespace.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 115280 [details] [diff] [review]
patch

>+  nsCOMPtr<nsIStyleContext> sc;
>+  nsresult rv = ResolveStyleContextFor(aContent, nsnull, getter_AddRefs(sc));
>+  NS_ENSURE_SUCCESS(rv, rv);

That should be (as of bryner's landing today):

nsRefPtr<nsStyleContext> sc = ResolveStyleContextFor(aContent, nsnull);
NS_ENSURE_SUCCESS(sc, NS_ERROR_FAILURE);

r+sr=bzbarsky with that change.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

shouldn't that be NS_ENSURE_TRUE rather than _FAILURE? |sc| is a pointer, not a
nsresult...

Comment 17

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I wrote this patch in a 1.3-branch tree, but when I move it to the trunk I'll do
that.

Comment 18

[Boris Zbarsky [:bzbarsky]]

er, yes.  I was thinking NS_ENSURE_TRUE and writing NS_ENSURE_SUCCESS... ;)

Comment 19

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Fix checked in to trunk, 2003-02-23 09:20 PST.  Fix checked in to
MOZILLA_1_3_BRANCH, 2003-02-24 13:16 PST.

Comment 20

[Dennis Daniels]

*** Bug 195763 has been marked as a duplicate of this bug. ***