394042 - Dangling pointer in nsXULPDGlobalObject leads to mem corruption/crashes

Status: RESOLVED FIXED

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

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

nsXULPDGlobalObject::ClearGlobalObjectOwner doesn't null out mGlobalObjectOwner and leads to this (I think), which is the #9 topcrash on windows at the moment:

bp-7a8bc2d4-5516-11dc-a69b-001a4bd43e5c

Looks like trouble from bug 389911.

bz and Purify helped me find this.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Null it out!

Fix?

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 278610 [details] [diff] [review]
Null it out!

Hmm, interesting. Looks like the right thing to do no doubt, but bug 389911 didn't really change this AFAICT from looking at the patch, SetGlobalObjectOwner(), which was replaced by ClearGlobalObjectOwner(), didn't null out mGlobalObjectOwner either.

Either way, r+sr+a=jst

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed. Thanks!

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm.  Before that patch landed we had nsXULPrototypeDocument::~nsXULPrototypeDocument doing:

-        mGlobalObject->SetGlobalObjectOwner(nsnull);

and nsXULPDGlobalObject::SetGlobalObjectOwner indeed never set mGlobalObjectOwner.  I'm not sure just clearing the owner is right; the right fix imo would be to cache the owner's current principal so GetPrincipal() doesn't suddenly start returning null and _then_ clear the owner.  Otherwise we're silently changing the security context of this global, which is almost worse than crashing...

Comment 5

[Boris Zbarsky [:bzbarsky]]

I _would_ like to know why this broke, by the way, since bug 389911 doesn't seem to obviously be it.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Reopening based on comment 4.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Cache the principal

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 278713 [details] [diff] [review]
Cache the principal

No.  You don't want to save the cached principal until the script global object owner is going away.  Up until that point you want to use that owner's principal, no matter what.

Note also that if |this == nsXULPrototypeDocument::gSystemGlobal| you should never need to do anything with the cached principal.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Cache the principal, v2

Like this?

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Cache the principal, v3

Actually, you probably meant like this.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 278723 [details] [diff] [review]
Cache the principal, v3

>Index: content/xul/document/src/nsXULPrototypeDocument.cpp
>+        if (mCachedPrincipal) {
>+            return mCachedPrincipal;
>+        }
>         return nsnull;

That's equivalent to:

  return mCachedPrincipal;

With that change, r+sr=bzbarsky

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Cache the principal, v4

Same patch with that change.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Cool, all fixed up.