Bug 98624 – Null-check for DOMWindow before feeding it to FeatureObserver.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Mike West]

Null-check for DOMWindow before feeding it to FeatureObserver.

[Mike West]

Created attachment 167485 [details]
Patch

[Geoffrey Garen]

Comment on attachment 167485 [details]
Patch

Is it possible to write a regression test for this?

How did you discover this issue?

[Mike West]

Created attachment 167486 [details]
Patch

[Mike West]

Hey Adam! This patch follows up on http://crbug.com/154484. WDYT?

[Mike West]

(In reply to comment #2)
> (From update of attachment 167485 [details])
> Is it possible to write a regression test for this?

I'll see what I can do. The bug would only occur when a document without a frame instantiates a Content Security Policy. Adam suggested that this might happen in documents created via XMLHttpRequest, so I'll start there...

> How did you discover this issue?

Crash report in http://crbug.com/154484

[Mike West]

Created attachment 167489 [details]
Patch

[Mike West]

(In reply to comment #2)
> (From update of attachment 167485 [details])
> Is it possible to write a regression test for this?

The latest patch has a test that crashes reliably without the patch. Thanks for the nudge.

[Adam Barth]

Comment on attachment 167489 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167489&action=review

> Source/WebCore/page/FeatureObserver.cpp:58
> +    if (!domWindow)
> +        return;

Presumably we don't need this null check in both places.  It's better to have it in ContentSecurityPolicy.cpp since most callers will already know that the DOMWindow is non-0.

[Mike West]

(In reply to comment #8)
> (From update of attachment 167489 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167489&action=review
> 
> > Source/WebCore/page/FeatureObserver.cpp:58
> > +    if (!domWindow)
> > +        return;
> 
> Presumably we don't need this null check in both places.  It's better to have it in ContentSecurityPolicy.cpp since most callers will already know that the DOMWindow is non-0.

I agree that the check is redundant, but wouldn't it be better to have it in FeatureObserver? It wasn't obvious to me that I was creating a crasher at the time I wrote the code; I'd rather gracefully handle such oversight.

If you'd prefer that it remain in the caller, how would you feel about an ASSERT in FeatureObserver? If we're going to crash, we might as well mean it. :)

[Adam Barth]

Sure.  An assert is fine.

[Mike West]

Created attachment 167499 [details]
Assert.

[Adam Barth]

Comment on attachment 167499 [details]
Assert.

Thanks.

[WebKit Review Bot]

Comment on attachment 167499 [details]
Assert.

Clearing flags on attachment: 167499

Committed r130657: <http://trac.webkit.org/changeset/130657>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.