SSIA
Created attachment 237881 [details] Patch
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.
Created attachment 237884 [details] Patch
(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().
(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 on attachment 237884 [details] Patch Clearing flags on attachment: 237884 Committed r173458: <http://trac.webkit.org/changeset/173458>
All reviewed patches have been landed. Closing bug.
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.