From d7ccda56e2b2e10310485dfaed5890b8890c90de Mon Sep 17 00:00:00 2001 From: roytam1 Date: Thu, 10 Feb 2022 11:05:08 +0800 Subject: [PATCH] import changes from `dev' branch of rmottola/Arctic-Fox: - Bug 622657 - catch invalid bookmark items and delete them when syncing bookmarks. r=rnewman (bd91e14c9a) - Bug 1166853 - Sync "hangs" when trying to apply a livemark to a reconciled folder. r=rnewman (94a6a19753) - Bug 1012597 - Part 1: provide a way to invalidate the Places GUIDs cache. r=rnewman (66f43cb831) - Bug 1012597 - Part 2: ensure Sync invalidates the Places GUIDs cache when needed. r=rnewman (2a67b0dfa8) - Bug 1182366 - avoid an invalid bookmark from preventing all bookmarks syncing. r=rnewman (df895d4c35) - let -> var (e9c382c761) - Bug 1188170 - log the url string when the Sync bookmarks engine fails to get a URI. r=rnewman (424e5405f7) - Bug 1195603 - prevent Sync from blocking app shutdown. r=rnewman (94c3091f9c) - Bug 1183934 - only log an error saving JSON if an error actually occurred. r=rnewman (6b4358035c) - Bug 1198385 - Use MFBT guard macros in the editor guard objects; r=froydnj (72e5e9f66f) - Bug 1198385 follow-up: Fix the build bustage on a CLOSED TREE (d626e56e92) - Bug 1177013 - Use CancelCurrentTransaction to avoid crashes (r=dvander) (362e5fc343) - Bug 1154990 - Better error messages when IPC send fails (r=bent) (b311fb94aa) - Bug 1177013 - Avoid memory leaks when returning errors from IPC Send (r=dvander) (f453a8feb5) - Bug 1176096 - Ensure we don't do self-moves in move assignment (r=bent) (31aca4ad89) - Bug 1177013 - Crash in IPC situations where we don't know what to do (r=dvander) (0e86cc437e) - Bug 1185639 - Allow deferred message processing to happen between consecutive IPC message dispatches. r=jimm (7782c9caaf) --- editor/libeditor/nsEditorUtils.cpp | 13 +- editor/libeditor/nsEditorUtils.h | 52 ++++-- ipc/glue/MessageChannel.cpp | 155 +++++++++++----- ipc/glue/MessageChannel.h | 10 +- ipc/glue/Neutering.h | 64 +++++++ ipc/glue/WindowsMessageLoop.cpp | 173 +++++++++--------- ipc/glue/moz.build | 1 + js/ipc/WrapperOwner.cpp | 2 +- services/cloudsync/CloudSyncPlacesWrapper.jsm | 17 -- services/common/async.js | 13 +- services/sync/modules/browserid_identity.js | 2 +- services/sync/modules/engines.js | 18 +- services/sync/modules/engines/bookmarks.js | 75 +++++++- services/sync/modules/engines/history.js | 2 +- services/sync/modules/engines/passwords.js | 3 +- services/sync/modules/identity.js | 3 +- services/sync/modules/record.js | 3 +- services/sync/modules/resource.js | 4 +- .../sync/tests/unit/test_bookmark_invalid.js | 111 +++++++++++ services/sync/tests/unit/xpcshell.ini | 1 + toolkit/components/places/PlacesUtils.jsm | 53 +++++- ...est_PlacesUtils_invalidateCachedGuidFor.js | 24 +++ .../components/places/tests/unit/xpcshell.ini | 1 + widget/windows/nsWindow.cpp | 2 +- xpcom/threads/nsThread.cpp | 5 +- 25 files changed, 609 insertions(+), 198 deletions(-) create mode 100644 ipc/glue/Neutering.h create mode 100644 services/sync/tests/unit/test_bookmark_invalid.js create mode 100644 toolkit/components/places/tests/unit/test_PlacesUtils_invalidateCachedGuidFor.js diff --git a/editor/libeditor/nsEditorUtils.cpp b/editor/libeditor/nsEditorUtils.cpp index b7c964e35a..5513419f53 100644 --- a/editor/libeditor/nsEditorUtils.cpp +++ b/editor/libeditor/nsEditorUtils.cpp @@ -31,9 +31,11 @@ using namespace mozilla::dom; * nsAutoSelectionReset *****************************************************************************/ -nsAutoSelectionReset::nsAutoSelectionReset(Selection* aSel, nsEditor* aEd) +nsAutoSelectionReset::nsAutoSelectionReset(Selection* aSel, nsEditor* aEd + MOZ_GUARD_OBJECT_NOTIFIER_PARAM_IN_IMPL) : mSel(nullptr), mEd(nullptr) { + MOZ_GUARD_OBJECT_NOTIFIER_INIT; if (!aSel || !aEd) return; // not much we can do, bail. if (aEd->ArePreservingSelection()) return; // we already have initted mSavedSel, so this must be nested call. mSel = aSel; @@ -66,8 +68,9 @@ nsAutoSelectionReset::Abort() * some helper classes for iterating the dom tree *****************************************************************************/ -nsDOMIterator::nsDOMIterator(nsINode& aNode) +nsDOMIterator::nsDOMIterator(nsINode& aNode MOZ_GUARD_OBJECT_NOTIFIER_PARAM_IN_IMPL) { + MOZ_GUARD_OBJECT_NOTIFIER_INIT; mIter = NS_NewContentIterator(); DebugOnly res = mIter->Init(&aNode); MOZ_ASSERT(NS_SUCCEEDED(res)); @@ -80,8 +83,9 @@ nsDOMIterator::Init(nsRange& aRange) return mIter->Init(&aRange); } -nsDOMIterator::nsDOMIterator() +nsDOMIterator::nsDOMIterator(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM_IN_IMPL) { + MOZ_GUARD_OBJECT_NOTIFIER_INIT; } nsDOMIterator::~nsDOMIterator() @@ -102,7 +106,8 @@ nsDOMIterator::AppendList(const nsBoolDomIterFunctor& functor, } } -nsDOMSubtreeIterator::nsDOMSubtreeIterator() +nsDOMSubtreeIterator::nsDOMSubtreeIterator(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM_IN_IMPL) + : nsDOMIterator(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM_TO_PARENT) { } diff --git a/editor/libeditor/nsEditorUtils.h b/editor/libeditor/nsEditorUtils.h index f4c987d0a6..0d8248174e 100644 --- a/editor/libeditor/nsEditorUtils.h +++ b/editor/libeditor/nsEditorUtils.h @@ -14,6 +14,7 @@ #include "nsIDOMNode.h" #include "nsIEditor.h" #include "nscore.h" +#include "mozilla/GuardObjects.h" class nsIAtom; class nsIContentIterator; @@ -34,10 +35,22 @@ class MOZ_STACK_CLASS nsAutoPlaceHolderBatch { private: nsCOMPtr mEd; + MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER public: - nsAutoPlaceHolderBatch( nsIEditor *aEd, nsIAtom *atom) : mEd(do_QueryInterface(aEd)) - { if (mEd) mEd->BeginPlaceHolderTransaction(atom); } - ~nsAutoPlaceHolderBatch() { if (mEd) mEd->EndPlaceHolderTransaction(); } + nsAutoPlaceHolderBatch(nsIEditor *aEd, nsIAtom *atom MOZ_GUARD_OBJECT_NOTIFIER_PARAM) + : mEd(do_QueryInterface(aEd)) + { + MOZ_GUARD_OBJECT_NOTIFIER_INIT; + if (mEd) { + mEd->BeginPlaceHolderTransaction(atom); + } + } + ~nsAutoPlaceHolderBatch() + { + if (mEd) { + mEd->EndPlaceHolderTransaction(); + } + } }; /*************************************************************************** @@ -47,8 +60,13 @@ class MOZ_STACK_CLASS nsAutoPlaceHolderBatch */ class MOZ_STACK_CLASS nsAutoEditBatch : public nsAutoPlaceHolderBatch { + MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER public: - explicit nsAutoEditBatch( nsIEditor *aEd) : nsAutoPlaceHolderBatch(aEd,nullptr) {} + explicit nsAutoEditBatch(nsIEditor *aEd MOZ_GUARD_OBJECT_NOTIFIER_PARAM) + : nsAutoPlaceHolderBatch(aEd, nullptr) + { + MOZ_GUARD_OBJECT_NOTIFIER_INIT; + } ~nsAutoEditBatch() {} }; @@ -62,10 +80,11 @@ class MOZ_STACK_CLASS nsAutoSelectionReset /** ref-counted reference to the selection that we are supposed to restore */ nsRefPtr mSel; nsEditor *mEd; // non-owning ref to nsEditor + MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER public: /** constructor responsible for remembering all state needed to restore aSel */ - nsAutoSelectionReset(mozilla::dom::Selection* aSel, nsEditor* aEd); + nsAutoSelectionReset(mozilla::dom::Selection* aSel, nsEditor* aEd MOZ_GUARD_OBJECT_NOTIFIER_PARAM); /** destructor restores mSel to its former state */ ~nsAutoSelectionReset(); @@ -82,9 +101,11 @@ class MOZ_STACK_CLASS nsAutoRules public: nsAutoRules(nsEditor *ed, EditAction action, - nsIEditor::EDirection aDirection) : - mEd(ed), mDoNothing(false) + nsIEditor::EDirection aDirection + MOZ_GUARD_OBJECT_NOTIFIER_PARAM) + : mEd(ed), mDoNothing(false) { + MOZ_GUARD_OBJECT_NOTIFIER_INIT; if (mEd && !mEd->mAction) // mAction will already be set if this is nested call { mEd->StartOperation(action, aDirection); @@ -102,6 +123,7 @@ class MOZ_STACK_CLASS nsAutoRules protected: nsEditor *mEd; bool mDoNothing; + MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER }; @@ -113,8 +135,10 @@ class MOZ_STACK_CLASS nsAutoTxnsConserveSelection { public: - explicit nsAutoTxnsConserveSelection(nsEditor *ed) : mEd(ed), mOldState(true) + explicit nsAutoTxnsConserveSelection(nsEditor *ed MOZ_GUARD_OBJECT_NOTIFIER_PARAM) + : mEd(ed), mOldState(true) { + MOZ_GUARD_OBJECT_NOTIFIER_INIT; if (mEd) { mOldState = mEd->GetShouldTxnSetSelection(); @@ -133,6 +157,7 @@ class MOZ_STACK_CLASS nsAutoTxnsConserveSelection protected: nsEditor *mEd; bool mOldState; + MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER }; /*************************************************************************** @@ -142,8 +167,9 @@ class MOZ_STACK_CLASS nsAutoUpdateViewBatch { public: - explicit nsAutoUpdateViewBatch(nsEditor *ed) : mEd(ed) + explicit nsAutoUpdateViewBatch(nsEditor *ed MOZ_GUARD_OBJECT_NOTIFIER_PARAM) : mEd(ed) { + MOZ_GUARD_OBJECT_NOTIFIER_INIT; NS_ASSERTION(mEd, "null mEd pointer!"); if (mEd) @@ -158,6 +184,7 @@ class MOZ_STACK_CLASS nsAutoUpdateViewBatch protected: nsEditor *mEd; + MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER }; /****************************************************************************** @@ -173,9 +200,9 @@ class nsBoolDomIterFunctor class MOZ_STACK_CLASS nsDOMIterator { public: - nsDOMIterator(); + explicit nsDOMIterator(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM); - explicit nsDOMIterator(nsINode& aNode); + explicit nsDOMIterator(nsINode& aNode MOZ_GUARD_OBJECT_NOTIFIER_PARAM); virtual ~nsDOMIterator(); nsresult Init(nsRange& aRange); @@ -184,12 +211,13 @@ class MOZ_STACK_CLASS nsDOMIterator nsTArray>& arrayOfNodes) const; protected: nsCOMPtr mIter; + MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER }; class MOZ_STACK_CLASS nsDOMSubtreeIterator : public nsDOMIterator { public: - nsDOMSubtreeIterator(); + explicit nsDOMSubtreeIterator(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM); virtual ~nsDOMSubtreeIterator(); nsresult Init(nsRange& aRange); diff --git a/ipc/glue/MessageChannel.cpp b/ipc/glue/MessageChannel.cpp index 22ac6b3500..b5d0faea16 100644 --- a/ipc/glue/MessageChannel.cpp +++ b/ipc/glue/MessageChannel.cpp @@ -105,7 +105,7 @@ struct RunnableMethodTraits DebugAbort(__FILE__, __LINE__, #_cond,## __VA_ARGS__); \ } while (0) -static bool gParentIsBlocked; +static MessageChannel* gParentProcessBlocker; namespace mozilla { namespace ipc { @@ -166,7 +166,7 @@ public: InterruptFrame& operator=(InterruptFrame&& aOther) { - MOZ_ASSERT(&aOther != this); + MOZ_RELEASE_ASSERT(&aOther != this); this->~InterruptFrame(); new (this) InterruptFrame(mozilla::Move(aOther)); return *this; @@ -403,6 +403,10 @@ MessageChannel::Clear() // In practice, mListener owns the channel, so the channel gets deleted // before mListener. But just to be safe, mListener is a weak pointer. + if (gParentProcessBlocker == this) { + gParentProcessBlocker = nullptr; + } + mDequeueOneTask->Cancel(); mWorkerLoop = nullptr; @@ -527,7 +531,7 @@ MessageChannel::Echo(Message* aMsg) MonitorAutoLock lock(*mMonitor); if (!Connected()) { - ReportConnectionError("MessageChannel"); + ReportConnectionError("MessageChannel", msg); return false; } @@ -550,13 +554,28 @@ MessageChannel::Send(Message* aMsg) MonitorAutoLock lock(*mMonitor); if (!Connected()) { - ReportConnectionError("MessageChannel"); + ReportConnectionError("MessageChannel", msg); return false; } mLink->SendMessage(msg.forget()); return true; } +class CancelMessage : public IPC::Message +{ +public: + CancelMessage() : + IPC::Message(MSG_ROUTING_NONE, CANCEL_MESSAGE_TYPE, PRIORITY_NORMAL) + { + } + static bool Read(const Message* msg) { + return true; + } + void Log(const std::string& aPrefix, FILE* aOutf) const { + fputs("(special `Cancel' message)", aOutf); + } +}; + bool MessageChannel::MaybeInterceptSpecialIOMessage(const Message& aMsg) { @@ -776,9 +795,51 @@ MessageChannel::ProcessPendingRequests() } } +bool +MessageChannel::WasTransactionCanceled(int transaction, int prio) +{ + if (transaction == mCurrentTransaction) { + return false; + } + + // This isn't an assert so much as an intentional crash because we're in a + // situation that we don't know how to recover from: The child is awaiting + // a reply to a normal-priority sync message. The transaction that this + // message initiated has now been canceled. That could only happen if a CPOW + // raced with the sync message and was dispatched by the child while the + // child was awaiting the sync reply; at some point while dispatching the + // CPOW, the transaction was canceled. + // + // Notes: + // + // 1. We don't want to cancel the normal-priority sync message along with + // the CPOWs because the browser relies on these messages working + // reliably. + // + // 2. Ideally we would like to avoid dispatching CPOWs while awaiting a sync + // response. This isn't possible though. To avoid deadlock, the parent would + // have to dispatch the sync message while waiting for the CPOW + // response. However, it wouldn't have dispatched async messages at that + // time, so we would have a message ordering bug. Dispatching the async + // messages first causes other hard-to-handle situations (what if they send + // CPOWs?). + // + // 3. We would like to be able to cancel the CPOWs but not the sync + // message. However, that would leave both the parent and the child running + // code at the same time, all while the sync message is still + // outstanding. That can cause a problem where message replies are received + // out of order. + IPC_ASSERT(prio != IPC::Message::PRIORITY_NORMAL, + "Intentional crash: We canceled a CPOW that was racing with a sync message."); + + return true; +} + bool MessageChannel::Send(Message* aMsg, Message* aReply) { + nsAutoPtr msg(aMsg); + // See comment in DispatchSyncMessage. MaybeScriptBlocker scriptBlocker(this, true); @@ -791,9 +852,10 @@ MessageChannel::Send(Message* aMsg, Message* aReply) #ifdef OS_WIN SyncStackFrame frame(this, false); + NeuteredWindowRegion neuteredRgn(mFlags & REQUIRE_DEFERRED_MESSAGE_PROTECTION); #endif - CxxStackFrame f(*this, OUT_MESSAGE, aMsg); + CxxStackFrame f(*this, OUT_MESSAGE, msg); MonitorAutoLock lock(*mMonitor); @@ -806,17 +868,27 @@ MessageChannel::Send(Message* aMsg, Message* aReply) } if (DispatchingSyncMessagePriority() == IPC::Message::PRIORITY_NORMAL && - aMsg->priority() > IPC::Message::PRIORITY_NORMAL) + msg->priority() > IPC::Message::PRIORITY_NORMAL) { // Don't allow sending CPOWs while we're dispatching a sync message. // If you want to do that, use sendRpcMessage instead. return false; } - IPC_ASSERT(aMsg->is_sync(), "can only Send() sync messages here"); - IPC_ASSERT(aMsg->priority() >= DispatchingSyncMessagePriority(), + if (mCurrentTransaction && + (msg->priority() < DispatchingSyncMessagePriority() || + mAwaitingSyncReplyPriority > msg->priority() || + DispatchingSyncMessagePriority() == IPC::Message::PRIORITY_URGENT || + DispatchingAsyncMessagePriority() == IPC::Message::PRIORITY_URGENT)) + { + CancelCurrentTransactionInternal(); + mLink->SendMessage(new CancelMessage()); + } + + IPC_ASSERT(msg->is_sync(), "can only Send() sync messages here"); + IPC_ASSERT(msg->priority() >= DispatchingSyncMessagePriority(), "can't send sync message of a lesser priority than what's being dispatched"); - IPC_ASSERT(AwaitingSyncReplyPriority() <= aMsg->priority(), + IPC_ASSERT(AwaitingSyncReplyPriority() <= msg->priority(), "nested sync message sends must be of increasing priority"); IPC_ASSERT(DispatchingSyncMessagePriority() != IPC::Message::PRIORITY_URGENT, @@ -824,10 +896,8 @@ MessageChannel::Send(Message* aMsg, Message* aReply) IPC_ASSERT(DispatchingAsyncMessagePriority() != IPC::Message::PRIORITY_URGENT, "not allowed to send messages while dispatching urgent messages"); - nsAutoPtr msg(aMsg); - if (!Connected()) { - ReportConnectionError("MessageChannel::SendAndWait"); + ReportConnectionError("MessageChannel::SendAndWait", msg); return false; } @@ -845,8 +915,7 @@ MessageChannel::Send(Message* aMsg, Message* aReply) msg->set_transaction_id(transaction); ProcessPendingRequests(); - if (mCurrentTransaction != transaction) { - // Transaction was canceled when dispatching. + if (WasTransactionCanceled(transaction, prio)) { return false; } @@ -854,8 +923,7 @@ MessageChannel::Send(Message* aMsg, Message* aReply) while (true) { ProcessPendingRequests(); - if (mCurrentTransaction != transaction) { - // Transaction was canceled when dispatching. + if (WasTransactionCanceled(transaction, prio)) { return false; } @@ -878,8 +946,7 @@ MessageChannel::Send(Message* aMsg, Message* aReply) return false; } - if (mCurrentTransaction != transaction) { - // Transaction was canceled by other side. + if (WasTransactionCanceled(transaction, prio)) { return false; } @@ -925,6 +992,7 @@ MessageChannel::Call(Message* aMsg, Message* aReply) #ifdef OS_WIN SyncStackFrame frame(this, true); + NeuteredWindowRegion neuteredRgn(mFlags & REQUIRE_DEFERRED_MESSAGE_PROTECTION); #endif // This must come before MonitorAutoLock, as its destructor acquires the @@ -939,7 +1007,7 @@ MessageChannel::Call(Message* aMsg, Message* aReply) MonitorAutoLock lock(*mMonitor); if (!Connected()) { - ReportConnectionError("MessageChannel::Call"); + ReportConnectionError("MessageChannel::Call", aMsg); return false; } @@ -969,6 +1037,12 @@ MessageChannel::Call(Message* aMsg, Message* aReply) return false; } +#ifdef OS_WIN + /* We should pump messages at this point to ensure that the IPC peer + does not become deadlocked on a pending inter-thread SendMessage() */ + neuteredRgn.PumpOnce(); +#endif + // Now might be the time to process a message deferred because of race // resolution. MaybeUndeferIncall(); @@ -1085,6 +1159,7 @@ MessageChannel::WaitForIncomingMessage() { #ifdef OS_WIN SyncStackFrame frame(this, true); + NeuteredWindowRegion neuteredRgn(mFlags & REQUIRE_DEFERRED_MESSAGE_PROTECTION); #endif { // Scope for lock @@ -1260,8 +1335,8 @@ MessageChannel::DispatchSyncMessage(const Message& aMsg, Message*& aReply) IPC_ASSERT(prio >= mAwaitingSyncReplyPriority, "dispatching a message of lower priority while waiting for a response"); - bool dummy; - bool& blockingVar = ShouldBlockScripts() ? gParentIsBlocked : dummy; + MessageChannel* dummy; + MessageChannel*& blockingVar = ShouldBlockScripts() ? gParentProcessBlocker : dummy; Result rv; if (mTimedOutMessageSeqno && mTimedOutMessagePriority >= prio) { @@ -1280,7 +1355,7 @@ MessageChannel::DispatchSyncMessage(const Message& aMsg, Message*& aReply) // for a response to its urgent message). rv = MsgNotAllowed; } else { - AutoSetValue blocked(blockingVar, true); + AutoSetValue blocked(blockingVar, this); AutoSetValue sync(mDispatchingSyncMessage, true); AutoSetValue prioSet(mDispatchingSyncMessagePriority, prio); rv = mListener->OnMessageReceived(aMsg, aReply); @@ -1594,7 +1669,7 @@ MessageChannel::ReportMessageRouteError(const char* channelName) const } void -MessageChannel::ReportConnectionError(const char* aChannelName) const +MessageChannel::ReportConnectionError(const char* aChannelName, Message* aMsg) const { AssertWorkerThread(); mMonitor->AssertCurrentThreadOwns(); @@ -1621,7 +1696,16 @@ MessageChannel::ReportConnectionError(const char* aChannelName) const NS_RUNTIMEABORT("unreached"); } - PrintErrorMessage(mSide, aChannelName, errorMsg); + if (aMsg) { + char reason[512]; + PR_snprintf(reason, sizeof(reason), + "(msgtype=0x%lX,name=%s) %s", + aMsg->type(), aMsg->name(), errorMsg); + + PrintErrorMessage(mSide, aChannelName, reason); + } else { + PrintErrorMessage(mSide, aChannelName, errorMsg); + } MonitorAutoUnlock unlock(*mMonitor); mListener->OnProcessingError(MsgDropped, errorMsg); @@ -1941,21 +2025,6 @@ MessageChannel::GetTopmostMessageRoutingId() const return frame.GetRoutingId(); } -class CancelMessage : public IPC::Message -{ -public: - CancelMessage() : - IPC::Message(MSG_ROUTING_NONE, CANCEL_MESSAGE_TYPE, PRIORITY_NORMAL) - { - } - static bool Read(const Message* msg) { - return true; - } - void Log(const std::string& aPrefix, FILE* aOutf) const { - fputs("(special `Cancel' message)", aOutf); - } -}; - void MessageChannel::CancelCurrentTransactionInternal() { @@ -1981,10 +2050,12 @@ MessageChannel::CancelCurrentTransaction() mLink->SendMessage(new CancelMessage()); } -bool -ParentProcessIsBlocked() +void +CancelCPOWs() { - return gParentIsBlocked; + if (gParentProcessBlocker) { + gParentProcessBlocker->CancelCurrentTransaction(); + } } } // namespace ipc diff --git a/ipc/glue/MessageChannel.h b/ipc/glue/MessageChannel.h index 4f3fa2665a..22b51f8d84 100644 --- a/ipc/glue/MessageChannel.h +++ b/ipc/glue/MessageChannel.h @@ -15,6 +15,9 @@ #include "mozilla/Monitor.h" #include "mozilla/Vector.h" #include "mozilla/WeakPtr.h" +#if defined(OS_WIN) +#include "mozilla/ipc/Neutering.h" +#endif // defined(OS_WIN) #include "mozilla/ipc/Transport.h" #include "MessageLink.h" #include "nsAutoPtr.h" @@ -217,7 +220,7 @@ class MessageChannel : HasResultCodes void PostErrorNotifyTask(); void OnNotifyMaybeChannelError(); - void ReportConnectionError(const char* aChannelName) const; + void ReportConnectionError(const char* aChannelName, Message* aMsg = nullptr) const; void ReportMessageRouteError(const char* channelName) const; bool MaybeHandleError(Result code, const Message& aMsg, const char* channelName); @@ -405,6 +408,7 @@ class MessageChannel : HasResultCodes // Tell the IO thread to close the channel and wait for it to ACK. void SynchronouslyClose(); + bool WasTransactionCanceled(int transaction, int prio); bool ShouldDeferMessage(const Message& aMsg); void OnMessageReceivedFromLink(const Message& aMsg); void OnChannelErrorFromLink(); @@ -731,8 +735,8 @@ class MessageChannel : HasResultCodes int32_t mPeerPid; }; -bool -ParentProcessIsBlocked(); +void +CancelCPOWs(); } // namespace ipc } // namespace mozilla diff --git a/ipc/glue/Neutering.h b/ipc/glue/Neutering.h new file mode 100644 index 0000000000..ea1a2bf5e9 --- /dev/null +++ b/ipc/glue/Neutering.h @@ -0,0 +1,64 @@ +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ +/* vim: set ts=8 sts=2 et sw=2 tw=80: */ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#ifndef mozilla_ipc_Neutering_h +#define mozilla_ipc_Neutering_h + +#include "mozilla/GuardObjects.h" + +/** + * This header declares RAII wrappers for Window neutering. See + * WindowsMessageLoop.cpp for more details. + */ + +namespace mozilla { +namespace ipc { + +/** + * This class is a RAII wrapper around Window neutering. As long as a + * NeuteredWindowRegion object is instantiated, Win32 windows belonging to the + * current thread will be neutered. It is safe to nest multiple instances of + * this class. + */ +class MOZ_STACK_CLASS NeuteredWindowRegion +{ +public: + explicit NeuteredWindowRegion(bool aDoNeuter MOZ_GUARD_OBJECT_NOTIFIER_PARAM); + ~NeuteredWindowRegion(); + + /** + * This function clears any backlog of nonqueued messages that are pending for + * the current thread. + */ + void PumpOnce(); + +private: + MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER + bool mNeuteredByThis; +}; + +/** + * This class is analagous to MutexAutoUnlock for Mutex; it is an RAII class + * that is to be instantiated within a NeuteredWindowRegion, thus temporarily + * disabling neutering for the remainder of its enclosing block. + * @see NeuteredWindowRegion + */ +class MOZ_STACK_CLASS DeneuteredWindowRegion +{ +public: + DeneuteredWindowRegion(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM); + ~DeneuteredWindowRegion(); + +private: + MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER + bool mReneuter; +}; + +} // namespace ipc +} // namespace mozilla + +#endif // mozilla_ipc_Neutering_h + diff --git a/ipc/glue/WindowsMessageLoop.cpp b/ipc/glue/WindowsMessageLoop.cpp index 3f7afaaa18..9bf21abcd7 100644 --- a/ipc/glue/WindowsMessageLoop.cpp +++ b/ipc/glue/WindowsMessageLoop.cpp @@ -8,6 +8,7 @@ #include "mozilla/DebugOnly.h" #include "WindowsMessageLoop.h" +#include "Neutering.h" #include "MessageChannel.h" #include "nsAutoPtr.h" @@ -862,6 +863,81 @@ IsTimeoutExpired(PRIntervalTime aStart, PRIntervalTime aTimeout) (aTimeout <= (PR_IntervalNow() - aStart)); } +static HHOOK gWindowHook; + +static inline void +StartNeutering() +{ + MOZ_ASSERT(gUIThreadId); + MOZ_ASSERT(!gWindowHook); + NS_ASSERTION(!MessageChannel::IsPumpingMessages(), + "Shouldn't be pumping already!"); + MessageChannel::SetIsPumpingMessages(true); + gWindowHook = ::SetWindowsHookEx(WH_CALLWNDPROC, CallWindowProcedureHook, + nullptr, gUIThreadId); + NS_ASSERTION(gWindowHook, "Failed to set hook!"); +} + +static void +StopNeutering() +{ + MOZ_ASSERT(MessageChannel::IsPumpingMessages()); + ::UnhookWindowsHookEx(gWindowHook); + gWindowHook = NULL; + ::UnhookNeuteredWindows(); + // Before returning we need to set a hook to run any deferred messages that + // we received during the IPC call. The hook will unset itself as soon as + // someone else calls GetMessage, PeekMessage, or runs code that generates + // a "nonqueued" message. + ::ScheduleDeferredMessageRun(); + MessageChannel::SetIsPumpingMessages(false); +} + +NeuteredWindowRegion::NeuteredWindowRegion(bool aDoNeuter MOZ_GUARD_OBJECT_NOTIFIER_PARAM_IN_IMPL) + : mNeuteredByThis(!gWindowHook) +{ + MOZ_GUARD_OBJECT_NOTIFIER_INIT; + if (aDoNeuter && mNeuteredByThis) { + StartNeutering(); + } +} + +NeuteredWindowRegion::~NeuteredWindowRegion() +{ + if (gWindowHook && mNeuteredByThis) { + StopNeutering(); + } +} + +void +NeuteredWindowRegion::PumpOnce() +{ + MSG msg = {0}; + // Pump any COM messages so that we don't hang due to STA marshaling. + if (gCOMWindow && ::PeekMessageW(&msg, gCOMWindow, 0, 0, PM_REMOVE)) { + ::TranslateMessage(&msg); + ::DispatchMessageW(&msg); + } + // Expunge any nonqueued messages on the current thread. + ::PeekMessageW(&msg, nullptr, 0, 0, PM_NOREMOVE); +} + +DeneuteredWindowRegion::DeneuteredWindowRegion(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM_IN_IMPL) + : mReneuter(gWindowHook != NULL) +{ + MOZ_GUARD_OBJECT_NOTIFIER_INIT; + if (mReneuter) { + StopNeutering(); + } +} + +DeneuteredWindowRegion::~DeneuteredWindowRegion() +{ + if (mReneuter) { + StartNeutering(); + } +} + bool MessageChannel::WaitForSyncNotify() { @@ -916,15 +992,6 @@ MessageChannel::WaitForSyncNotify() NS_ASSERTION(timerId, "SetTimer failed!"); } - // Setup deferred processing of native events while we wait for a response. - NS_ASSERTION(!MessageChannel::IsPumpingMessages(), - "Shouldn't be pumping already!"); - - MessageChannel::SetIsPumpingMessages(true); - HHOOK windowHook = SetWindowsHookEx(WH_CALLWNDPROC, CallWindowProcedureHook, - nullptr, gUIThreadId); - NS_ASSERTION(windowHook, "Failed to set hook!"); - { while (1) { MSG msg = { 0 }; @@ -998,25 +1065,11 @@ MessageChannel::WaitForSyncNotify() } } - // Unhook the neutered window procedure hook. - UnhookWindowsHookEx(windowHook); - - // Unhook any neutered windows procedures so messages can be delivered - // normally. - UnhookNeuteredWindows(); - - // Before returning we need to set a hook to run any deferred messages that - // we received during the IPC call. The hook will unset itself as soon as - // someone else calls GetMessage, PeekMessage, or runs code that generates - // a "nonqueued" message. - ScheduleDeferredMessageRun(); - if (timerId) { KillTimer(nullptr, timerId); + timerId = 0; } - MessageChannel::SetIsPumpingMessages(false); - return WaitResponse(timedout); } @@ -1050,56 +1103,28 @@ MessageChannel::WaitForInterruptNotify() UINT_PTR timerId = 0; TimeoutData timeoutData = { 0 }; - // windowHook is used as a flag variable for the loop below: if it is set + // gWindowHook is used as a flag variable for the loop below: if it is set // and we start to spin a nested event loop, we need to clear the hook and // process deferred/pending messages. - // If windowHook is nullptr, MessageChannel::IsPumpingMessages should be false. - HHOOK windowHook = nullptr; - while (1) { - NS_ASSERTION((!!windowHook) == MessageChannel::IsPumpingMessages(), - "windowHook out of sync with reality"); + NS_ASSERTION((!!gWindowHook) == MessageChannel::IsPumpingMessages(), + "gWindowHook out of sync with reality"); if (mTopFrame->mSpinNestedEvents) { - if (windowHook) { - UnhookWindowsHookEx(windowHook); - windowHook = nullptr; - - if (timerId) { - KillTimer(nullptr, timerId); - timerId = 0; - } - - // Used by widget to assert on incoming native events - MessageChannel::SetIsPumpingMessages(false); - - // Unhook any neutered windows procedures so messages can be delievered - // normally. - UnhookNeuteredWindows(); - - // Send all deferred "nonqueued" message to the intended receiver. - // We're dropping into SpinInternalEventLoop so we should be fairly - // certain these will get delivered soohn. - ScheduleDeferredMessageRun(); + if (gWindowHook && timerId) { + KillTimer(nullptr, timerId); + timerId = 0; } + DeneuteredWindowRegion deneuteredRgn; SpinInternalEventLoop(); ResetEvent(mEvent); return true; } - if (!windowHook) { - MessageChannel::SetIsPumpingMessages(true); - windowHook = SetWindowsHookEx(WH_CALLWNDPROC, CallWindowProcedureHook, - nullptr, gUIThreadId); - NS_ASSERTION(windowHook, "Failed to set hook!"); - - NS_ASSERTION(!timerId, "Timer already initialized?"); - - if (mTimeoutMs != kNoTimeout) { - InitTimeoutData(&timeoutData, mTimeoutMs); - timerId = SetTimer(nullptr, 0, mTimeoutMs, nullptr); - NS_ASSERTION(timerId, "SetTimer failed!"); - } + if (mTimeoutMs != kNoTimeout && !timerId) { + InitTimeoutData(&timeoutData, mTimeoutMs); + timerId = SetTimer(nullptr, 0, mTimeoutMs, nullptr); + NS_ASSERTION(timerId, "SetTimer failed!"); } MSG msg = { 0 }; @@ -1151,27 +1176,11 @@ MessageChannel::WaitForInterruptNotify() } } - if (windowHook) { - // Unhook the neutered window procedure hook. - UnhookWindowsHookEx(windowHook); - - // Unhook any neutered windows procedures so messages can be delivered - // normally. - UnhookNeuteredWindows(); - - // Before returning we need to set a hook to run any deferred messages that - // we received during the IPC call. The hook will unset itself as soon as - // someone else calls GetMessage, PeekMessage, or runs code that generates - // a "nonqueued" message. - ScheduleDeferredMessageRun(); - - if (timerId) { - KillTimer(nullptr, timerId); - } + if (timerId) { + KillTimer(nullptr, timerId); + timerId = 0; } - MessageChannel::SetIsPumpingMessages(false); - return WaitResponse(timedout); } diff --git a/ipc/glue/moz.build b/ipc/glue/moz.build index 97f41930d1..e5ff21a00b 100644 --- a/ipc/glue/moz.build +++ b/ipc/glue/moz.build @@ -25,6 +25,7 @@ EXPORTS.mozilla.ipc += [ 'IOThreadChild.h', 'MessageChannel.h', 'MessageLink.h', + 'Neutering.h', 'ProcessChild.h', 'ProtocolUtils.h', 'ScopedXREEmbed.h', diff --git a/js/ipc/WrapperOwner.cpp b/js/ipc/WrapperOwner.cpp index 3fb1491e6b..1f1db4f234 100644 --- a/js/ipc/WrapperOwner.cpp +++ b/js/ipc/WrapperOwner.cpp @@ -1031,7 +1031,7 @@ WrapperOwner::ActorDestroy(ActorDestroyReason why) bool WrapperOwner::ipcfail(JSContext* cx) { - JS_ReportError(cx, "child process crashed or timedout"); + JS_ReportError(cx, "cross-process JS call failed"); return false; } diff --git a/services/cloudsync/CloudSyncPlacesWrapper.jsm b/services/cloudsync/CloudSyncPlacesWrapper.jsm index cdddb9a4b2..7606541b01 100644 --- a/services/cloudsync/CloudSyncPlacesWrapper.jsm +++ b/services/cloudsync/CloudSyncPlacesWrapper.jsm @@ -80,23 +80,6 @@ PlacesWrapper.prototype = { return deferred.promise; }, - setGuidForLocalId: function (localId, guid) { - let deferred = Promise.defer(); - - let stmt = "UPDATE moz_bookmarks " + - "SET guid = :guid " + - "WHERE id = :item_id"; - let query = this.placesQueries.getQuery(stmt); - - query.params.guid = guid; - query.params.item_id = localId; - - this.asyncQuery(query) - .then(deferred.resolve, deferred.reject); - - return deferred.promise; - }, - getItemsById: function (ids, types) { let deferred = Promise.defer(); let stmt = "SELECT b.id, b.type, b.parent, b.position, b.title, b.guid, b.dateAdded, b.lastModified, p.url " + diff --git a/services/common/async.js b/services/common/async.js index 5bdfc8a547..444b3c98a2 100644 --- a/services/common/async.js +++ b/services/common/async.js @@ -123,13 +123,24 @@ this.Async = { Services.obs.addObserver(function onQuitApplication() { Services.obs.removeObserver(onQuitApplication, "quit-application"); Async.checkAppReady = function() { - throw Components.Exception("App. Quitting", Cr.NS_ERROR_ABORT); + let exception = Components.Exception("App. Quitting", Cr.NS_ERROR_ABORT); + exception.appIsShuttingDown = true; + throw exception; }; }, "quit-application", false); // In the common case, checkAppReady just returns true return (Async.checkAppReady = function() { return true; })(); }, + /** + * Check if the passed exception is one raised by checkAppReady. Typically + * this will be used in exception handlers to allow such exceptions to + * make their way to the top frame and allow the app to actually terminate. + */ + isShutdownException(exception) { + return exception && exception.appIsShuttingDown === true; + }, + /** * Return the two things you need to make an asynchronous call synchronous * by spinning the event loop. diff --git a/services/sync/modules/browserid_identity.js b/services/sync/modules/browserid_identity.js index bc8ea6b308..b787bf5686 100644 --- a/services/sync/modules/browserid_identity.js +++ b/services/sync/modules/browserid_identity.js @@ -654,7 +654,7 @@ this.BrowserIDManager.prototype = { this._ensureValidToken().then(cb, cb); try { cb.wait(); - } catch (ex) { + } catch (ex if !Async.isShutdownException(ex)) { this._log.error("Failed to fetch a token for authentication", ex); return null; } diff --git a/services/sync/modules/engines.js b/services/sync/modules/engines.js index eabddae2bb..8880cc26fa 100644 --- a/services/sync/modules/engines.js +++ b/services/sync/modules/engines.js @@ -307,7 +307,7 @@ Store.prototype = { // originating exception. // ex.cause will carry its stack with it when rethrown. throw ex.cause; - } catch (ex) { + } catch (ex if !Async.isShutdownException(ex)) { this._log.warn("Failed to apply incoming record " + record.id); this._log.warn("Encountered exception: " + Utils.exceptionStr(ex)); failed.push(record.id); @@ -788,7 +788,11 @@ SyncEngine.prototype = { return this._toFetch; }, set toFetch(val) { - let cb = (error) => this._log.error(Utils.exceptionStr(error)); + let cb = (error) => { + if (error) { + this._log.error("Failed to read JSON records to fetch", error); + } + } // Coerce the array to a string for more efficient comparison. if (val + "" == this._toFetch) { return; @@ -993,7 +997,7 @@ SyncEngine.prototype = { this._tracker.ignoreAll = true; try { failed = failed.concat(this._store.applyIncomingBatch(applyBatch)); - } catch (ex) { + } catch (ex if !Async.isShutdownException(ex)) { // Catch any error that escapes from applyIncomingBatch. At present // those will all be abort events. this._log.warn("Got exception " + Utils.exceptionStr(ex) + @@ -1087,7 +1091,7 @@ SyncEngine.prototype = { self._log.warn("Reconciliation failed: aborting incoming processing."); failed.push(item.id); aborting = ex.cause; - } catch (ex) { + } catch (ex if !Async.isShutdownException(ex)) { self._log.warn("Failed to reconcile incoming record " + item.id); self._log.warn("Encountered exception: " + Utils.exceptionStr(ex)); failed.push(item.id); @@ -1459,8 +1463,7 @@ SyncEngine.prototype = { out.encrypt(this.service.collectionKeys.keyForCollection(this.name)); up.pushData(out); - } - catch(ex) { + } catch (ex if !Async.isShutdownException(ex)) { this._log.warn("Error creating record: " + Utils.exceptionStr(ex)); } @@ -1551,8 +1554,7 @@ SyncEngine.prototype = { try { this._log.trace("Trying to decrypt a record from the server.."); test.get(); - } - catch(ex) { + } catch (ex if !Async.isShutdownException(ex)) { this._log.debug("Failed test decrypt: " + Utils.exceptionStr(ex)); } diff --git a/services/sync/modules/engines/bookmarks.js b/services/sync/modules/engines/bookmarks.js index 1936afc3fc..269c38e1c1 100644 --- a/services/sync/modules/engines/bookmarks.js +++ b/services/sync/modules/engines/bookmarks.js @@ -135,7 +135,7 @@ BookmarkSeparator.prototype = { Utils.deferGetSet(BookmarkSeparator, "cleartext", "pos"); -let kSpecialIds = { +var kSpecialIds = { // Special IDs. Note that mobile can attempt to create a record on // dereference; special accessors are provided to prevent recursion within @@ -240,6 +240,32 @@ BookmarksEngine.prototype = { } }, + // A diagnostic helper to get the string value for a bookmark's URL given + // its ID. Always returns a string - on error will return a string in the + // form of "" as this is purely for, eg, logging. + // (This means hitting the DB directly and we don't bother using a cached + // statement - we should rarely hit this.) + _getStringUrlForId(id) { + let url; + try { + let stmt = this._store._getStmt(` + SELECT h.url + FROM moz_places h + JOIN moz_bookmarks b ON h.id = b.fk + WHERE b.id = :id`); + stmt.params.id = id; + let rows = Async.querySpinningly(stmt, ["url"]); + url = rows.length == 0 ? "" : rows[0].url; + } catch (ex if !Async.isShutdownException(ex)) { + if (ex instanceof Ci.mozIStorageError) { + url = ``; + } else { + url = ``; + } + } + return url; + }, + _guidMapFailed: false, _buildGUIDMap: function _buildGUIDMap() { let guidMap = {}; @@ -247,7 +273,19 @@ BookmarksEngine.prototype = { // Figure out with which key to store the mapping. let key; let id = this._store.idForGUID(guid); - switch (PlacesUtils.bookmarks.getItemType(id)) { + let itemType; + try { + itemType = PlacesUtils.bookmarks.getItemType(id); + } catch (ex) { + this._log.warn("Deleting invalid bookmark record with id", id); + try { + PlacesUtils.bookmarks.removeItem(id); + } catch (ex) { + this._log.warn("Failed to delete invalid bookmark", ex); + } + continue; + } + switch (itemType) { case PlacesUtils.bookmarks.TYPE_BOOKMARK: // Smart bookmarks map to their annotation value. @@ -256,12 +294,27 @@ BookmarksEngine.prototype = { queryId = PlacesUtils.annotations.getItemAnnotation( id, SMART_BOOKMARKS_ANNO); } catch(ex) {} - - if (queryId) + + if (queryId) { key = "q" + queryId; - else - key = "b" + PlacesUtils.bookmarks.getBookmarkURI(id).spec + ":" + - PlacesUtils.bookmarks.getItemTitle(id); + } else { + let uri; + try { + uri = PlacesUtils.bookmarks.getBookmarkURI(id); + } catch (ex) { + // Bug 1182366 - NS_ERROR_MALFORMED_URI here stops bookmarks sync. + // Try and get the string value of the URL for diagnostic purposes. + let url = this._getStringUrlForId(id); + this._log.warn(`Deleting bookmark with invalid URI. url="${url}", id=${id}`); + try { + PlacesUtils.bookmarks.removeItem(id); + } catch (ex) { + this._log.warn("Failed to delete invalid bookmark", ex); + } + continue; + } + key = "b" + uri.spec + ":" + PlacesUtils.bookmarks.getItemTitle(id); + } break; case PlacesUtils.bookmarks.TYPE_FOLDER: key = "f" + PlacesUtils.bookmarks.getItemTitle(id); @@ -388,7 +441,7 @@ BookmarksEngine.prototype = { let guidMap; try { guidMap = this._buildGUIDMap(); - } catch (ex) { + } catch (ex if !Async.isShutdownException(ex)) { this._log.warn("Got exception \"" + Utils.exceptionStr(ex) + "\" building GUID map." + " Skipping all other incoming items."); @@ -759,7 +812,10 @@ BookmarksStore.prototype = { guid: record.id}; PlacesUtils.livemarks.addLivemark(livemarkObj).then( aLivemark => { spinningCb(null, [Components.results.NS_OK, aLivemark]) }, - () => { spinningCb(null, [Components.results.NS_ERROR_UNEXPECTED, aLivemark]) } + ex => { + this._log.error("creating livemark failed: " + ex); + spinningCb(null, [Components.results.NS_ERROR_UNEXPECTED, null]) + } ); let [status, livemark] = spinningCb.wait(); @@ -1139,6 +1195,7 @@ BookmarksStore.prototype = { stmt.params.guid = guid; stmt.params.item_id = id; Async.querySpinningly(stmt); + PlacesUtils.invalidateCachedGuidFor(id); return guid; }, diff --git a/services/sync/modules/engines/history.js b/services/sync/modules/engines/history.js index 99ecb45064..db156785c1 100644 --- a/services/sync/modules/engines/history.js +++ b/services/sync/modules/engines/history.js @@ -203,7 +203,7 @@ HistoryStore.prototype = { } else { shouldApply = this._recordToPlaceInfo(record); } - } catch(ex) { + } catch (ex if !Async.isShutdownException(ex)) { failed.push(record.id); shouldApply = false; } diff --git a/services/sync/modules/engines/passwords.js b/services/sync/modules/engines/passwords.js index 994b597678..e01b01b934 100644 --- a/services/sync/modules/engines/passwords.js +++ b/services/sync/modules/engines/passwords.js @@ -10,6 +10,7 @@ Cu.import("resource://services-sync/record.js"); Cu.import("resource://services-sync/constants.js"); Cu.import("resource://services-sync/engines.js"); Cu.import("resource://services-sync/util.js"); +Cu.import("resource://services-common/async.js"); this.LoginRec = function LoginRec(collection, id) { CryptoWrapper.call(this, collection, id); @@ -66,7 +67,7 @@ PasswordEngine.prototype = { // record success. Svc.Prefs.set("deletePwdFxA", true); Svc.Prefs.reset("deletePwd"); // The old prefname we previously used. - } catch (ex) { + } catch (ex if !Async.isShutdownException(ex)) { this._log.debug("Password deletes failed: " + Utils.exceptionStr(ex)); } } diff --git a/services/sync/modules/identity.js b/services/sync/modules/identity.js index 2bee13b5b8..ec5a541573 100644 --- a/services/sync/modules/identity.js +++ b/services/sync/modules/identity.js @@ -13,6 +13,7 @@ Cu.import("resource://gre/modules/Promise.jsm"); Cu.import("resource://services-sync/constants.js"); Cu.import("resource://gre/modules/Log.jsm"); Cu.import("resource://services-sync/util.js"); +Cu.import("resource://services-common/async.js"); // Lazy import to prevent unnecessary load on startup. for (let symbol of ["BulkKeyBundle", "SyncKeyBundle"]) { @@ -457,7 +458,7 @@ IdentityManager.prototype = { // cache. try { service.recordManager.get(service.storageURL + "meta/fxa_credentials"); - } catch (ex) { + } catch (ex if !Async.isShutdownException(ex)) { this._log.warn("Failed to pre-fetch the migration sentinel", ex); } }, diff --git a/services/sync/modules/record.js b/services/sync/modules/record.js index 04ccd2dd2e..6bdea7ce70 100644 --- a/services/sync/modules/record.js +++ b/services/sync/modules/record.js @@ -23,6 +23,7 @@ Cu.import("resource://services-sync/constants.js"); Cu.import("resource://services-sync/keys.js"); Cu.import("resource://services-sync/resource.js"); Cu.import("resource://services-sync/util.js"); +Cu.import("resource://services-common/async.js"); this.WBORecord = function WBORecord(collection, id) { this.data = {}; @@ -235,7 +236,7 @@ RecordManager.prototype = { record.deserialize(this.response); return this.set(url, record); - } catch(ex) { + } catch (ex if !Async.isShutdownException(ex)) { this._log.debug("Failed to import record: " + Utils.exceptionStr(ex)); return null; } diff --git a/services/sync/modules/resource.js b/services/sync/modules/resource.js index 1c2a67b902..ca4785406a 100644 --- a/services/sync/modules/resource.js +++ b/services/sync/modules/resource.js @@ -403,7 +403,7 @@ Resource.prototype = { try { this._doRequest(action, data, callback); return Async.waitForSyncCallback(cb); - } catch(ex) { + } catch (ex if !Async.isShutdownException(ex)) { // Combine the channel stack with this request stack. Need to create // a new error object for that. let error = Error(ex.message); @@ -556,7 +556,7 @@ ChannelListener.prototype = { try { this._onProgress(); - } catch (ex) { + } catch (ex if !Async.isShutdownException(ex)) { this._log.warn("Got exception calling onProgress handler during fetch of " + req.URI.spec); this._log.debug(CommonUtils.exceptionStr(ex)); diff --git a/services/sync/tests/unit/test_bookmark_invalid.js b/services/sync/tests/unit/test_bookmark_invalid.js new file mode 100644 index 0000000000..bd3b16478a --- /dev/null +++ b/services/sync/tests/unit/test_bookmark_invalid.js @@ -0,0 +1,111 @@ +Cu.import("resource://gre/modules/PlacesUtils.jsm"); +Cu.import("resource://gre/modules/Log.jsm"); +Cu.import("resource://gre/modules/Task.jsm"); +Cu.import("resource://services-sync/engines.js"); +Cu.import("resource://services-sync/engines/bookmarks.js"); +Cu.import("resource://services-sync/service.js"); +Cu.import("resource://services-sync/util.js"); + +Service.engineManager.register(BookmarksEngine); + +let engine = Service.engineManager.get("bookmarks"); +let store = engine._store; +let tracker = engine._tracker; + +// Return a promise resolved when the specified message is written to the +// bookmarks engine log. +function promiseLogMessage(messagePortion) { + return new Promise(resolve => { + let appender; + let log = Log.repository.getLogger("Sync.Engine.Bookmarks"); + + function TestAppender() { + Log.Appender.call(this); + } + TestAppender.prototype = Object.create(Log.Appender.prototype); + TestAppender.prototype.doAppend = function(message) { + if (message.indexOf(messagePortion) >= 0) { + log.removeAppender(appender); + resolve(); + } + }; + TestAppender.prototype.level = Log.Level.Debug; + appender = new TestAppender(); + log.addAppender(appender); + }); +} + +// Returns a promise that resolves if the specified ID does *not* exist and +// rejects if it does. +function promiseNoItem(itemId) { + return new Promise((resolve, reject) => { + try { + PlacesUtils.bookmarks.getFolderIdForItem(itemId); + reject("fetching the item didn't fail"); + } catch (ex) { + if (ex.result == Cr.NS_ERROR_ILLEGAL_VALUE) { + resolve("item doesn't exist"); + } else { + reject("unexpected exception: " + ex); + } + } + }); +} + +add_task(function* test_ignore_invalid_uri() { + _("Ensure that we don't die with invalid bookmarks."); + + // First create a valid bookmark. + let bmid = PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId, + Services.io.newURI("http://example.com/", null, null), + PlacesUtils.bookmarks.DEFAULT_INDEX, + "the title"); + + // Now update moz_places with an invalid url. + yield PlacesUtils.withConnectionWrapper("test_ignore_invalid_uri", Task.async(function* (db) { + yield db.execute( + `UPDATE moz_places SET url = :url + WHERE id = (SELECT b.fk FROM moz_bookmarks b + WHERE b.id = :id LIMIT 1)`, + { id: bmid, url: "" }); + })); + + // DB is now "corrupt" - setup a log appender to capture what we log. + let promiseMessage = promiseLogMessage('Deleting bookmark with invalid URI. url=""'); + // This should work and log our invalid id. + engine._buildGUIDMap(); + yield promiseMessage; + // And we should have deleted the item. + yield promiseNoItem(bmid); +}); + +add_task(function* test_ignore_missing_uri() { + _("Ensure that we don't die with a bookmark referencing an invalid bookmark id."); + + // First create a valid bookmark. + let bmid = PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId, + Services.io.newURI("http://example.com/", null, null), + PlacesUtils.bookmarks.DEFAULT_INDEX, + "the title"); + + // Now update moz_bookmarks to reference a non-existing places ID + yield PlacesUtils.withConnectionWrapper("test_ignore_missing_uri", Task.async(function* (db) { + yield db.execute( + `UPDATE moz_bookmarks SET fk = 999999 + WHERE id = :id` + , { id: bmid }); + })); + + // DB is now "corrupt" - bookmarks will fail to locate a string url to log + // and use "" as a placeholder. + let promiseMessage = promiseLogMessage('Deleting bookmark with invalid URI. url=""'); + engine._buildGUIDMap(); + yield promiseMessage; + // And we should have deleted the item. + yield promiseNoItem(bmid); +}); + +function run_test() { + initTestLogging('Trace'); + run_next_test(); +} diff --git a/services/sync/tests/unit/xpcshell.ini b/services/sync/tests/unit/xpcshell.ini index dc33c0eb28..eaf28ae552 100644 --- a/services/sync/tests/unit/xpcshell.ini +++ b/services/sync/tests/unit/xpcshell.ini @@ -136,6 +136,7 @@ run-sequentially = Hardcoded port in static files. [test_addons_tracker.js] [test_bookmark_batch_fail.js] [test_bookmark_engine.js] +[test_bookmark_invalid.js] [test_bookmark_legacy_microsummaries_support.js] [test_bookmark_livemarks.js] [test_bookmark_order.js] diff --git a/toolkit/components/places/PlacesUtils.jsm b/toolkit/components/places/PlacesUtils.jsm index 49eda1ee72..477ca068d8 100644 --- a/toolkit/components/places/PlacesUtils.jsm +++ b/toolkit/components/places/PlacesUtils.jsm @@ -1512,7 +1512,9 @@ this.PlacesUtils = { * @resolves to the GUID. * @rejects if aItemId is invalid. */ - promiseItemGuid: function (aItemId) GuidHelper.getItemGuid(aItemId), + promiseItemGuid(aItemId) { + return GuidHelper.getItemGuid(aItemId) + }, /** * Get the item id for an item (a bookmark, a folder or a separator) given @@ -1520,11 +1522,23 @@ this.PlacesUtils = { * * @param aGuid * an item GUID - * @retrun {Promise} + * @return {Promise} * @resolves to the GUID. * @rejects if there's no item for the given GUID. */ - promiseItemId: function (aGuid) GuidHelper.getItemId(aGuid), + promiseItemId(aGuid) { + return GuidHelper.getItemId(aGuid) + }, + + /** + * Invalidate the GUID cache for the given itemId. + * + * @param aItemId + * an item id + */ + invalidateCachedGuidFor(aItemId) { + GuidHelper.invalidateCacheForItemId(aItemId) + }, /** * Asynchronously retrieve a JS-object representation of a places bookmarks @@ -1612,7 +1626,7 @@ this.PlacesUtils = { item.id = itemId; // Cache it for promiseItemId consumers regardless. - GuidHelper.idsForGuids.set(item.guid, itemId); + GuidHelper.updateCache(itemId, item.guid); let type = aRow.getResultByName("type"); if (type == Ci.nsINavBookmarksService.TYPE_BOOKMARK) @@ -1916,7 +1930,7 @@ let GuidHelper = { this.ensureObservingRemovedItems(); let itemId = rows[0].getResultByName("id"); - this.idsForGuids.set(aGuid, itemId); + this.updateCache(itemId, aGuid); return itemId; }), @@ -1934,10 +1948,31 @@ let GuidHelper = { this.ensureObservingRemovedItems(); let guid = rows[0].getResultByName("guid"); - this.guidsForIds.set(aItemId, guid); + this.updateCache(aItemId, guid); return guid; }), + /** + * Updates the cache. + * + * @note This is the only place where the cache should be populated, + * invalidation relies on both Maps being populated at the same time. + */ + updateCache(aItemId, aGuid) { + if (typeof(aItemId) != "number" || aItemId <= 0) + throw new Error("Trying to update the GUIDs cache with an invalid itemId"); + if (typeof(aGuid) != "string" || !/^[a-zA-Z0-9\-_]{12}$/.test(aGuid)) + throw new Error("Trying to update the GUIDs cache with an invalid GUID"); + this.guidsForIds.set(aItemId, aGuid); + this.idsForGuids.set(aGuid, aItemId); + }, + + invalidateCacheForItemId(aItemId) { + let guid = this.guidsForIds.get(aItemId); + this.guidsForIds.delete(aItemId); + this.idsForGuids.delete(guid); + }, + ensureObservingRemovedItems: function () { if (!("observer" in this)) { /** @@ -1950,14 +1985,14 @@ let GuidHelper = { this.observer = { onItemAdded: (aItemId, aParentId, aIndex, aItemType, aURI, aTitle, aDateAdded, aGuid, aParentGuid) => { - this.guidsForIds.set(aItemId, aGuid); - this.guidsForIds.set(aParentId, aParentGuid); + this.updateCache(aItemId, aGuid); + this.updateCache(aParentId, aParentGuid); }, onItemRemoved: (aItemId, aParentId, aIndex, aItemTyep, aURI, aGuid, aParentGuid) => { this.guidsForIds.delete(aItemId); this.idsForGuids.delete(aGuid); - this.guidsForIds.set(aParentId, aParentGuid); + this.updateCache(aParentId, aParentGuid); }, QueryInterface: XPCOMUtils.generateQI(Ci.nsINavBookmarkObserver), diff --git a/toolkit/components/places/tests/unit/test_PlacesUtils_invalidateCachedGuidFor.js b/toolkit/components/places/tests/unit/test_PlacesUtils_invalidateCachedGuidFor.js new file mode 100644 index 0000000000..d42d59e5d9 --- /dev/null +++ b/toolkit/components/places/tests/unit/test_PlacesUtils_invalidateCachedGuidFor.js @@ -0,0 +1,24 @@ +add_task(function* () { + do_print("Add a bookmark."); + let bm = yield PlacesUtils.bookmarks.insert({ url: "http://example.com/", + parentGuid: PlacesUtils.bookmarks.unfiledGuid }); + let id = yield PlacesUtils.promiseItemId(bm.guid); + Assert.equal((yield PlacesUtils.promiseItemGuid(id)), bm.guid); + + // Ensure invalidating a non-existent itemId doesn't throw. + PlacesUtils.invalidateCachedGuidFor(null); + PlacesUtils.invalidateCachedGuidFor(9999); + + do_print("Change the GUID."); + let db = yield PlacesUtils.promiseWrappedConnection(); + yield db.execute("UPDATE moz_bookmarks SET guid = :guid WHERE id = :id", + { guid: "123456789012", id}); + // The cache should still point to the wrong id. + Assert.equal((yield PlacesUtils.promiseItemGuid(id)), bm.guid); + + do_print("Invalidate the cache."); + PlacesUtils.invalidateCachedGuidFor(id); + Assert.equal((yield PlacesUtils.promiseItemGuid(id)), "123456789012"); + Assert.equal((yield PlacesUtils.promiseItemId("123456789012")), id); + yield Assert.rejects(PlacesUtils.promiseItemId(bm.guid), /no item found for the given GUID/); +}); diff --git a/toolkit/components/places/tests/unit/xpcshell.ini b/toolkit/components/places/tests/unit/xpcshell.ini index d815f91b5e..063d4b5e57 100644 --- a/toolkit/components/places/tests/unit/xpcshell.ini +++ b/toolkit/components/places/tests/unit/xpcshell.ini @@ -121,6 +121,7 @@ skip-if = true [test_placeURIs.js] [test_PlacesSearchAutocompleteProvider.js] [test_PlacesUtils_asyncGetBookmarkIds.js] +[test_PlacesUtils_invalidateCachedGuidFor.js] [test_PlacesUtils_lazyobservers.js] [test_placesTxn.js] [test_preventive_maintenance.js] diff --git a/widget/windows/nsWindow.cpp b/widget/windows/nsWindow.cpp index 198a6c3f4f..2bb63a79e4 100644 --- a/widget/windows/nsWindow.cpp +++ b/widget/windows/nsWindow.cpp @@ -4310,7 +4310,7 @@ inline static mozilla::HangMonitor::ActivityType ActivityTypeForMessage(UINT msg // and http://msdn.microsoft.com/en-us/library/ms633573%28VS.85%29.aspx LRESULT CALLBACK nsWindow::WindowProc(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam) { - MOZ_RELEASE_ASSERT(!ipc::ParentProcessIsBlocked()); + ipc::CancelCPOWs(); HangMonitor::NotifyActivity(ActivityTypeForMessage(msg)); diff --git a/xpcom/threads/nsThread.cpp b/xpcom/threads/nsThread.cpp index 2334486f90..35b9860b98 100644 --- a/xpcom/threads/nsThread.cpp +++ b/xpcom/threads/nsThread.cpp @@ -723,8 +723,9 @@ nsThread::ProcessNextEvent(bool aMayWait, bool* aResult) #if !defined(MOZILLA_XPCOMRT_API) // If we're on the main thread, we shouldn't be dispatching CPOWs. - MOZ_RELEASE_ASSERT(mIsMainThread != MAIN_THREAD || - !ipc::ParentProcessIsBlocked()); + if (mIsMainThread == MAIN_THREAD) { + ipc::CancelCPOWs(); + } #endif // !defined(MOZILLA_XPCOMRT_API) if (NS_WARN_IF(PR_GetCurrentThread() != mThread)) {