Commit b729a998 authored by haraken's avatar haraken Committed by Commit bot

Clear LifecycleObserver::m_context when LifecycleObserver::contextDestroyed gets called

This CL clears LifecycleObserver::m_context when LifecycleObserver::contextDestroyed gets called.
This means that LifecycleObserver::context() starts returning false after the context gets destroyed.

The new behavior makes a lot of more sense, but the problem is that some existing code is assuming that
LifecycleObserver::context() keeps returning the context even after the context gets destroyed.
This CL fixed the problematic code to make all layout tests pass, but I'm not sure if I've updated all of the problematic code
(because the coverage of the layout tests is not sufficient).
However, the worst thing that can happen is that code that had been assuming that LifecycleObserver::context()
returns a context even after the context gets destroyed will stop working (it may cause a null-deref crash).
I think the risk is low.

BUG=610176

Review-Url: https://codereview.chromium.org/2317483005
Cr-Commit-Position: refs/heads/master@{#419951}
parent 8e05df27
...@@ -3,7 +3,7 @@ Accessing FileReader when in a detached state. ...@@ -3,7 +3,7 @@ Accessing FileReader when in a detached state.
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
PASS fileReader.readAsArrayBuffer(new File(['hello'], 'world.html')) threw exception AbortError: Failed to execute 'readAsArrayBuffer' on 'FileReader': Reading from a Document-detached FileReader is not supported.. PASS fileReader.readAsArrayBuffer(new File(['hello'], 'world.html')) threw exception AbortError: Failed to execute 'readAsArrayBuffer' on 'FileReader': Reading from a detached FileReader is not supported..
PASS successfullyParsed is true PASS successfullyParsed is true
TEST COMPLETE TEST COMPLETE
......
...@@ -4,7 +4,7 @@ Performing mixed content checks in a detached setting shouldn't crash. ...@@ -4,7 +4,7 @@ Performing mixed content checks in a detached setting shouldn't crash.
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
PASS xhr.send() threw exception NetworkError: Failed to execute 'send' on 'XMLHttpRequest': Failed to load 'http://user:pass@127.0.0.1:8000/doesNotExist'.. PASS xhr.send() threw exception NetworkError: Failed to execute 'send' on 'XMLHttpRequest': Failed to load 'http://user:pass@127.0.0.1:8000/doesNotExist': Document is already detached..
PASS successfullyParsed is true PASS successfullyParsed is true
TEST COMPLETE TEST COMPLETE
......
...@@ -295,6 +295,7 @@ void XMLHttpRequest::initResponseDocument() ...@@ -295,6 +295,7 @@ void XMLHttpRequest::initResponseDocument()
bool isHTML = responseIsHTML(); bool isHTML = responseIsHTML();
if ((m_response.isHTTP() && !responseIsXML() && !isHTML) if ((m_response.isHTTP() && !responseIsXML() && !isHTML)
|| (isHTML && m_responseTypeCode == ResponseTypeDefault) || (isHTML && m_responseTypeCode == ResponseTypeDefault)
|| !getExecutionContext()
|| getExecutionContext()->isWorkerGlobalScope()) { || getExecutionContext()->isWorkerGlobalScope()) {
m_responseDocument = nullptr; m_responseDocument = nullptr;
return; return;
...@@ -410,7 +411,7 @@ void XMLHttpRequest::setTimeout(unsigned timeout, ExceptionState& exceptionState ...@@ -410,7 +411,7 @@ void XMLHttpRequest::setTimeout(unsigned timeout, ExceptionState& exceptionState
{ {
// FIXME: Need to trigger or update the timeout Timer here, if needed. http://webkit.org/b/98156 // FIXME: Need to trigger or update the timeout Timer here, if needed. http://webkit.org/b/98156
// XHR2 spec, 4.7.3. "This implies that the timeout attribute can be set while fetching is in progress. If that occurs it will still be measured relative to the start of fetching." // XHR2 spec, 4.7.3. "This implies that the timeout attribute can be set while fetching is in progress. If that occurs it will still be measured relative to the start of fetching."
if (getExecutionContext()->isDocument() && !m_async) { if (getExecutionContext() && getExecutionContext()->isDocument() && !m_async) {
exceptionState.throwDOMException(InvalidAccessError, "Timeouts cannot be set for synchronous requests made from a document."); exceptionState.throwDOMException(InvalidAccessError, "Timeouts cannot be set for synchronous requests made from a document.");
return; return;
} }
...@@ -435,7 +436,7 @@ void XMLHttpRequest::setResponseType(const String& responseType, ExceptionState& ...@@ -435,7 +436,7 @@ void XMLHttpRequest::setResponseType(const String& responseType, ExceptionState&
// Newer functionality is not available to synchronous requests in window contexts, as a spec-mandated // Newer functionality is not available to synchronous requests in window contexts, as a spec-mandated
// attempt to discourage synchronous XHR use. responseType is one such piece of functionality. // attempt to discourage synchronous XHR use. responseType is one such piece of functionality.
if (!m_async && getExecutionContext()->isDocument()) { if (getExecutionContext() && getExecutionContext()->isDocument() && !m_async) {
exceptionState.throwDOMException(InvalidAccessError, "The response type cannot be changed for synchronous requests made from a document."); exceptionState.throwDOMException(InvalidAccessError, "The response type cannot be changed for synchronous requests made from a document.");
return; return;
} }
...@@ -556,6 +557,9 @@ void XMLHttpRequest::setWithCredentials(bool value, ExceptionState& exceptionSta ...@@ -556,6 +557,9 @@ void XMLHttpRequest::setWithCredentials(bool value, ExceptionState& exceptionSta
void XMLHttpRequest::open(const AtomicString& method, const String& urlString, ExceptionState& exceptionState) void XMLHttpRequest::open(const AtomicString& method, const String& urlString, ExceptionState& exceptionState)
{ {
if (!getExecutionContext())
return;
KURL url(getExecutionContext()->completeURL(urlString)); KURL url(getExecutionContext()->completeURL(urlString));
if (!validateOpenArguments(method, url, exceptionState)) if (!validateOpenArguments(method, url, exceptionState))
return; return;
...@@ -565,6 +569,9 @@ void XMLHttpRequest::open(const AtomicString& method, const String& urlString, E ...@@ -565,6 +569,9 @@ void XMLHttpRequest::open(const AtomicString& method, const String& urlString, E
void XMLHttpRequest::open(const AtomicString& method, const String& urlString, bool async, const String& username, const String& password, ExceptionState& exceptionState) void XMLHttpRequest::open(const AtomicString& method, const String& urlString, bool async, const String& username, const String& password, ExceptionState& exceptionState)
{ {
if (!getExecutionContext())
return;
KURL url(getExecutionContext()->completeURL(urlString)); KURL url(getExecutionContext()->completeURL(urlString));
if (!validateOpenArguments(method, url, exceptionState)) if (!validateOpenArguments(method, url, exceptionState))
return; return;
...@@ -642,8 +649,11 @@ void XMLHttpRequest::open(const AtomicString& method, const KURL& url, bool asyn ...@@ -642,8 +649,11 @@ void XMLHttpRequest::open(const AtomicString& method, const KURL& url, bool asyn
bool XMLHttpRequest::initSend(ExceptionState& exceptionState) bool XMLHttpRequest::initSend(ExceptionState& exceptionState)
{ {
if (!getExecutionContext()) if (!getExecutionContext()) {
handleNetworkError();
throwForLoadFailureIfNeeded(exceptionState, "Document is already detached.");
return false; return false;
}
if (m_state != kOpened || m_loader) { if (m_state != kOpened || m_loader) {
exceptionState.throwDOMException(InvalidStateError, "The object's state must be OPENED."); exceptionState.throwDOMException(InvalidStateError, "The object's state must be OPENED.");
...@@ -876,7 +886,7 @@ void XMLHttpRequest::createRequest(PassRefPtr<EncodedFormData> httpBody, Excepti ...@@ -876,7 +886,7 @@ void XMLHttpRequest::createRequest(PassRefPtr<EncodedFormData> httpBody, Excepti
} }
DCHECK(getExecutionContext()); DCHECK(getExecutionContext());
ExecutionContext& executionContext = *this->getExecutionContext(); ExecutionContext& executionContext = *getExecutionContext();
// The presence of upload event listeners forces us to use preflighting because POSTing to an URL that does not // The presence of upload event listeners forces us to use preflighting because POSTing to an URL that does not
// permit cross origin requests should look exactly like POSTing to an URL that does not respond at all. // permit cross origin requests should look exactly like POSTing to an URL that does not respond at all.
...@@ -1504,7 +1514,7 @@ void XMLHttpRequest::endLoading() ...@@ -1504,7 +1514,7 @@ void XMLHttpRequest::endLoading()
changeState(kDone); changeState(kDone);
if (!getExecutionContext()->isDocument() || !document() || !document()->frame() || !document()->frame()->page()) if (!getExecutionContext() || !getExecutionContext()->isDocument() || !document() || !document()->frame() || !document()->frame()->page())
return; return;
if (status() >= 200 && status() < 300) { if (status() >= 200 && status() < 300) {
......
...@@ -211,7 +211,7 @@ bool Body::isBodyLocked() ...@@ -211,7 +211,7 @@ bool Body::isBodyLocked()
bool Body::hasPendingActivity() const bool Body::hasPendingActivity() const
{ {
if (getExecutionContext()->activeDOMObjectsAreStopped()) if (!getExecutionContext() || getExecutionContext()->activeDOMObjectsAreStopped())
return false; return false;
if (!bodyBuffer()) if (!bodyBuffer())
return false; return false;
......
...@@ -35,6 +35,8 @@ const char* ServiceWorkerContainerClient::supplementName() ...@@ -35,6 +35,8 @@ const char* ServiceWorkerContainerClient::supplementName()
ServiceWorkerContainerClient* ServiceWorkerContainerClient::from(ExecutionContext* context) ServiceWorkerContainerClient* ServiceWorkerContainerClient::from(ExecutionContext* context)
{ {
if (!context)
return nullptr;
if (context->isWorkerGlobalScope()) { if (context->isWorkerGlobalScope()) {
WorkerClients* clients = toWorkerGlobalScope(context)->clients(); WorkerClients* clients = toWorkerGlobalScope(context)->clients();
ASSERT(clients); ASSERT(clients);
......
...@@ -243,7 +243,8 @@ DOMWebSocket::~DOMWebSocket() ...@@ -243,7 +243,8 @@ DOMWebSocket::~DOMWebSocket()
void DOMWebSocket::logError(const String& message) void DOMWebSocket::logError(const String& message)
{ {
getExecutionContext()->addConsoleMessage(ConsoleMessage::create(JSMessageSource, ErrorMessageLevel, message)); if (getExecutionContext())
getExecutionContext()->addConsoleMessage(ConsoleMessage::create(JSMessageSource, ErrorMessageLevel, message));
} }
DOMWebSocket* DOMWebSocket::create(ExecutionContext* context, const String& url, ExceptionState& exceptionState) DOMWebSocket* DOMWebSocket::create(ExecutionContext* context, const String& url, ExceptionState& exceptionState)
......
...@@ -61,9 +61,7 @@ protected: ...@@ -61,9 +61,7 @@ protected:
{ {
} }
#if DCHECK_IS_ON()
T* context() { return static_cast<T*>(this); } T* context() { return static_cast<T*>(this); }
#endif
using ObserverSet = HeapHashSet<WeakMember<Observer>>; using ObserverSet = HeapHashSet<WeakMember<Observer>>;
...@@ -98,8 +96,9 @@ inline void LifecycleNotifier<T, Observer>::notifyContextDestroyed() ...@@ -98,8 +96,9 @@ inline void LifecycleNotifier<T, Observer>::notifyContextDestroyed()
ObserverSet observers; ObserverSet observers;
m_observers.swap(observers); m_observers.swap(observers);
for (Observer* observer : observers) { for (Observer* observer : observers) {
ASSERT(observer->lifecycleContext() == context()); DCHECK(observer->lifecycleContext() == context());
observer->contextDestroyed(); observer->contextDestroyed();
observer->clearContext();
} }
} }
......
...@@ -43,6 +43,11 @@ public: ...@@ -43,6 +43,11 @@ public:
Context* lifecycleContext() const { return m_lifecycleContext; } Context* lifecycleContext() const { return m_lifecycleContext; }
void clearContext()
{
setContext(nullptr);
}
protected: protected:
explicit LifecycleObserver(Context* context) explicit LifecycleObserver(Context* context)
: m_lifecycleContext(nullptr) : m_lifecycleContext(nullptr)
...@@ -52,11 +57,6 @@ protected: ...@@ -52,11 +57,6 @@ protected:
void setContext(Context*); void setContext(Context*);
void clearContext()
{
setContext(nullptr);
}
private: private:
WeakMember<Context> m_lifecycleContext; WeakMember<Context> m_lifecycleContext;
}; };
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment