Bug 136694 – Use toDocument instead of static_cast<Document*>

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Gyuyoung Kim]

SSIA

[Gyuyoung Kim]

Created attachment 237881 [details]
Patch

[Andy Estes]

Comment on attachment 237881 [details]
Patch

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

r=me, but I think you should fix the behavior change in XMLHttpRequest::document().

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1799
>      ASSERT(scriptExecutionContext()->isDocument());
> -    return *static_cast<Document*>(scriptExecutionContext());
> +    return *toDocument(scriptExecutionContext());

toDocument() calls ASSERT_WITH_SECURITY_IMPLICATION(!scriptExecutionContext() || scriptExecutionContext()->isDocument()), which is almost the same as the ASSERT above except with regard to how null is handled. I think you can simplify this code to just ASSERT that scriptExecutionContext() is non-null.

> Source/WebCore/Modules/webaudio/AudioContext.cpp:328
>      ASSERT(m_scriptExecutionContext && m_scriptExecutionContext->isDocument());
> -    return static_cast<Document*>(m_scriptExecutionContext);
> +    return toDocument(m_scriptExecutionContext);

Ditto.

> Source/WebCore/workers/DefaultSharedWorkerRepository.cpp:231
>      ASSERT_WITH_SECURITY_IMPLICATION(context->isDocument());
>      ASSERT(!isClosing());
>      MutexLocker lock(m_workerDocumentsLock);
> -    Document* document = static_cast<Document*>(context);
> -    m_workerDocuments.add(document);
> +    m_workerDocuments.add(toDocument(context));

Ditto.

> Source/WebCore/workers/WorkerMessagingProxy.cpp:86
>      ASSERT_WITH_SECURITY_IMPLICATION(m_scriptExecutionContext->isDocument());
> -    Document* document = static_cast<Document*>(m_scriptExecutionContext.get());
> +    Document* document = toDocument(m_scriptExecutionContext.get());

Ditto.

> Source/WebCore/xml/XMLHttpRequest.cpp:157
> -    ASSERT_WITH_SECURITY_IMPLICATION(scriptExecutionContext()->isDocument());
> -    return static_cast<Document*>(scriptExecutionContext());
> +    return toDocument(scriptExecutionContext());

You're introducing a behavior change by removing the ASSERT_WITH_SECURITY_IMPLICATION. Previously, if scriptExecutionContext() was null then we'd crash evaluating the ASSERT. Now we won't, since the ASSERT used by toDocument() allows null.

[Gyuyoung Kim]

Created attachment 237884 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #2)
> (From update of attachment 237881 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237881&action=review
> 
> r=me, but I think you should fix the behavior change in XMLHttpRequest::document().
> 
> > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1799
> >      ASSERT(scriptExecutionContext()->isDocument());
> > -    return *static_cast<Document*>(scriptExecutionContext());
> > +    return *toDocument(scriptExecutionContext());
> 
> toDocument() calls ASSERT_WITH_SECURITY_IMPLICATION(!scriptExecutionContext() || scriptExecutionContext()->isDocument()), which is almost the same as the ASSERT above except with regard to how null is handled. I think you can simplify this code to just ASSERT that scriptExecutionContext() is non-null.

Nice catching ! I fixed those null check. BTW, additionally one more thing is added to SCRIPT_EXECUTION_CONTEXT_TYPE_CASTS in order to remove .get().

[Gyuyoung Kim]

(In reply to comment #4)
 
> BTW, additionally one more thing is added to SCRIPT_EXECUTION_CONTEXT_TYPE_CASTS in order to remove .get().

If you don't mind to add it, I will land this patch.

[WebKit Commit Bot]

Comment on attachment 237884 [details]
Patch

Clearing flags on attachment: 237884

Committed r173458: <http://trac.webkit.org/changeset/173458>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andy Estes]

Comment on attachment 237884 [details]
Patch

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

> Source/WebCore/workers/DefaultSharedWorkerRepository.cpp:228
> -    ASSERT_WITH_SECURITY_IMPLICATION(context->isDocument());
> +    ASSERT_WITH_SECURITY_IMPLICATION(context);

I think this can be a regular ASSERT instead of ASSERT_WITH_SECURITY_IMPLICATION, since I don't believe there is a security implication to a null dereference (the security implication before had to do with bad casts).

> Source/WebCore/workers/WorkerMessagingProxy.cpp:86
> -    ASSERT_WITH_SECURITY_IMPLICATION(m_scriptExecutionContext->isDocument());
> -    Document* document = static_cast<Document*>(m_scriptExecutionContext.get());
> +    ASSERT_WITH_SECURITY_IMPLICATION(m_scriptExecutionContext);
> +    Document* document = toDocument(m_scriptExecutionContext);

Ditto.

> Source/WebCore/xml/XMLHttpRequest.cpp:158
> -    ASSERT_WITH_SECURITY_IMPLICATION(scriptExecutionContext()->isDocument());
> -    return static_cast<Document*>(scriptExecutionContext());
> +    ASSERT_WITH_SECURITY_IMPLICATION(scriptExecutionContext());
> +    return toDocument(scriptExecutionContext());

Ditto.