Bug 136694

Summary: Use toDocument instead of static_cast<Document*>
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: WebCore Misc.Assignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, calvaris, commit-queue, eric.carlson, esprehn+autocc, glenn, jer.noble, kangil.han, ltilve, philipj, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 136732    
Attachments:
Description Flags
Patch
none
Patch none

Description Gyuyoung Kim 2014-09-09 23:43:43 PDT 
SSIA
Comment 1 Gyuyoung Kim 2014-09-09 23:44:33 PDT 
Created attachment 237881 [details]
Patch
Comment 2 Andy Estes 2014-09-10 00:25:33 PDT 
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.
Comment 3 Gyuyoung Kim 2014-09-10 03:39:25 PDT 
Created attachment 237884 [details]
Patch
Comment 4 Gyuyoung Kim 2014-09-10 03:41:39 PDT 
(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().
Comment 5 Gyuyoung Kim 2014-09-10 03:46:47 PDT 
(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.
Comment 6 WebKit Commit Bot 2014-09-10 06:23:49 PDT 
Comment on attachment 237884 [details]
Patch

Clearing flags on attachment: 237884

Committed r173458: <http://trac.webkit.org/changeset/173458>
Comment 7 WebKit Commit Bot 2014-09-10 06:23:54 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 8 Andy Estes 2014-09-10 11:09:42 PDT 
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.