If this is a deadlock caused by a sync win message plugin -> browser, why didn't our deferral code pick it up and deal with it? I'm not sure what's going on here. *********************************************************************************** 423461e9-a965-4b85-b408-7ab742150425 Windows 7, 6.1.7601 Service Pack 1 NPSWF32_17_0_0_169.dll *********************************************************************************** PLUGIN ----------------------------------------------------------------------------------- 0 ZwOpenFile 1 DeleteFileW 2 SHDeleteFilePidl 3 CShellTask::~CShellTask() 4 CRemoveOperation::Do(IOleUndoManager *) 5 CCopyWorkItem::_DoOperation(CBaseOperation *) 6 CCopyWorkItem::_SetupAndPerformOp(ITransferSource *,int,int *,int *) 7 CCopyTree::AddTreeDone(int,int,unsigned __int64,int) 8 CCopyWorkItem::_ProcessChildren(int) 9 COpFromMatch::_CheckAndProcessItem(IShellFolder *,_ITEMID_CHILD const *,unsigned long) 10 CCopyWorkItem::_ProcessChildren(int) 11 COpFromMatch::_CheckAndProcessItem(IShellFolder *,_ITEMID_CHILD const *,unsigned long) 12 CRecursiveFolderOperation::Do(IOleUndoManager *) 13 CFileOperation::_EnumRootDo(CEnumOperation *) 14 CFileOperation::PrepareAndDoOperations() 15 SHFileOperationWithAdditionalFlags(_SHFILEOPSTRUCTW *,unsigned long) 16 SHFileOperationW 17 F_898302746_________________________________________________________ 18 F265811371__________________________________________________________ 19 NPP_ClearSiteData 20 mozilla::plugins::PluginModuleChild::AnswerNPP_ClearSiteData(nsCString const &,unsigned __int64 const &,unsigned __int64 const &,short *) src 21 mozilla::plugins::PPluginModuleChild::OnCallReceived(IPC::Message const &,IPC::Message * &) src 22 mozilla::ipc::MessageChannel::DispatchInterruptMessage(IPC::Message const &,unsigned int) src 23 mozilla::ipc::MessageChannel::OnMaybeDequeueOne() src 24 MessageLoop::DoWork() src 25 base::MessagePumpForUI::DoRunLoop() src 26 base::MessagePumpWin::RunWithDispatcher(base::MessagePump::Delegate *,base::MessagePumpWin::Dispatcher *) src 27 base::MessagePumpWin::Run(base::MessagePump::Delegate *) src 28 MessageLoop::RunInternal() src 29 MessageLoop::RunHandler() src 30 MessageLoop::Run() src 31 XRE_InitChildProcess src 32 content_process_main(int,char * * const) src 33 wmain src 34 __tmainCRTStartup 35 BaseThreadInitThunk 36 __RtlUserThreadStart 37 _RtlUserThreadStart CONTENT ----------------------------------------------------------------------------------- 0 ZwWaitForSingleObject 1 WaitForSingleObjectEx 2 WaitForSingleObjectExImplementation 3 WaitForSingleObject 4 PR_WaitCondVar src 5 mozilla::CondVar::Wait(unsigned int) src 6 mozilla::ipc::MessageChannel::WaitForSyncNotify() src 7 mozilla::ipc::MessageChannel::Send(IPC::Message *,IPC::Message *) src 8 mozilla::dom::PScreenManagerChild::SendScreenForBrowser(mozilla::dom::IdType<mozilla::dom::TabParent> const &,mozilla::dom::ScreenDetails *,bool *) src 9 nsScreenManagerProxy::ScreenForNativeWidget(void *,nsIScreen * *) src 10 nsDeviceContext::FindScreen(nsIScreen * *) src 11 nsDeviceContext::ComputeClientRectUsingScreen(nsRect *) src 12 nsDeviceContext::GetClientRect(nsRect &) src 13 nsScreen::GetAvailRect(nsRect &) src 14 nsScreen::GetAvailWidth(mozilla::ErrorResult &) src 15 mozilla::dom::ScreenBinding::get_availWidth src 16 mozilla::dom::GenericBindingGetter(JSContext *,unsigned int,JS::Value *) src 17 js::jit::DoCallNativeGetter src 18 js::NewDateObjectMsec(JSContext *,double) src BROWSER ----------------------------------------------------------------------------------- 0 NtWaitForMultipleObjects 1 WaitForMultipleObjectsEx 2 WaitForMultipleObjectsExImplementation 3 RealMsgWaitForMultipleObjectsEx 4 MsgWaitForMultipleObjects 5 mozilla::ipc::MessageChannel::WaitForInterruptNotify() src 6 mozilla::ipc::MessageChannel::Call(IPC::Message *,IPC::Message *) src 7 mozilla::plugins::PPluginModuleParent::CallNPP_ClearSiteData(nsCString const &,unsigned __int64 const &,unsigned __int64 const &,short *) src 8 mozilla::plugins::PluginModuleParent::NPP_ClearSiteData(char const *,unsigned __int64,unsigned __int64) src 9 nsPluginHost::ClearSiteData(nsIPluginTag *,nsACString_internal const &,unsigned __int64,__int64) src 10 NS_InvokeByIndex src 11 XPC_WN_CallMethod(JSContext *,unsigned int,JS::Value *) src 12 unknown: offset=unknown function=unknown 13 unknown: offset=unknown function=unknown 14 unknown: offset=unknown function=unknown 15 EnterBaseline src 16 js::jit::EnterBaselineAtBranch(JSContext *,js::InterpreterFrame *,unsigned char *) src 17 Interpret src 18 js::RunScript(JSContext *,js::RunState &) src 19 js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) src 20 Interpret src 21 js::RunScript(JSContext *,js::RunState &) src 22 js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) src 23 js::Invoke(JSContext *,JS::Value const &,JS::Value const &,unsigned int,JS::Value const *,JS::MutableHandle<JS::Value>) src 24 mozilla::dom::EventListener::HandleEvent(JSContext *,JS::Handle<JS::Value>,mozilla::dom::Event &,mozilla::ErrorResult &) src 25 mozilla::EventListenerManager::HandleEventInternal(nsPresContext *,mozilla::WidgetEvent *,nsIDOMEvent * *,mozilla::dom::EventTarget *,nsEventStatus *) src 26 mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem> &,mozilla::EventChainPostVisitor &,mozilla::EventDispatchingCallback *,mozilla::ELMCreationDetector &) src 27 mozilla::EventDispatcher::Dispatch(nsISupports *,nsPresContext *,mozilla::WidgetEvent *,nsIDOMEvent *,nsEventStatus *,mozilla::EventDispatchingCallback *,nsTArray<mozilla::dom::EventTarget *> *) src 28 mozilla::EventDispatcher::DispatchDOMEvent(nsISupports *,mozilla::WidgetEvent *,nsIDOMEvent *,nsPresContext *,nsEventStatus *) src 29 PresShell::HandleDOMEventWithTarget(nsIContent *,nsIDOMEvent *,nsEventStatus *) src 30 nsContentUtils::DispatchXULCommand(nsIContent *,bool,nsIDOMEvent *,nsIPresShell *,bool,bool,bool,bool) src 31 nsButtonBoxFrame::DoMouseClick(mozilla::WidgetGUIEvent *,bool) src 32 nsButtonBoxFrame::MouseClicked(nsPresContext *,mozilla::WidgetGUIEvent *) src 33 nsButtonBoxFrame::HandleEvent(nsPresContext *,mozilla::WidgetGUIEvent *,nsEventStatus *) src 34 nsPresShellEventCB::HandleEvent(mozilla::EventChainPostVisitor &) src 35 mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem> &,mozilla::EventChainPostVisitor &,mozilla::EventDispatchingCallback *,mozilla::ELMCreationDetector &) src 36 mozilla::EventDispatcher::Dispatch(nsISupports *,nsPresContext *,mozilla::WidgetEvent *,nsIDOMEvent *,nsEventStatus *,mozilla::EventDispatchingCallback *,nsTArray<mozilla::dom::EventTarget *> *) src 37 PresShell::DispatchEventToDOM(mozilla::WidgetEvent *,nsEventStatus *,nsPresShellEventCB *) src 38 PresShell::HandleEventInternal(mozilla::WidgetEvent *,nsEventStatus *) src 39 PresShell::HandleEventWithTarget(mozilla::WidgetEvent *,nsIFrame *,nsIContent *,nsEventStatus *) src 40 mozilla::EventStateManager::CheckForAndDispatchClick(nsPresContext *,mozilla::WidgetMouseEvent *,nsEventStatus *) src 41 mozilla::EventStateManager::PostHandleEvent(nsPresContext *,mozilla::WidgetEvent *,nsIFrame *,nsEventStatus *) src 42 PresShell::HandleEventInternal(mozilla::WidgetEvent *,nsEventStatus *) src 43 PresShell::HandlePositionedEvent(nsIFrame *,mozilla::WidgetGUIEvent *,nsEventStatus *) src 44 PresShell::HandleEvent(nsIFrame *,mozilla::WidgetGUIEvent *,bool,nsEventStatus *,nsIContent * *) src 45 nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent *,nsView *,nsEventStatus *) src 46 nsView::HandleEvent(mozilla::WidgetGUIEvent *,bool) src 47 nsWindow::DispatchEvent(mozilla::WidgetGUIEvent *,nsEventStatus &) src 48 nsBaseWidget::DispatchInputEvent(mozilla::WidgetInputEvent *) src 49 nsWindow::DispatchMouseEvent(unsigned int,unsigned int,long,bool,short,unsigned short) src 50 nsWindow::ProcessMessage(unsigned int,unsigned int &,long &,long *) src 51 nsWindow::WindowProcInternal(HWND__ *,unsigned int,unsigned int,long) src 52 CallWindowProcCrashProtected src 53 nsWindow::WindowProc(HWND__ *,unsigned int,unsigned int,long) src 54 InternalCallWinProc 55 UserCallWinProcCheckWow 56 DispatchMessageWorker 57 DispatchMessageW 58 nsAppShell::ProcessNextNativeEvent(bool) src 59 nsBaseAppShell::DoProcessNextNativeEvent(bool,unsigned int) src 60 nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal *,bool,unsigned int) src 61 nsThread::ProcessNextEvent(bool,bool *) src 62 NS_ProcessNextEvent(nsIThread *,bool) src 63 nsXULWindow::ShowModal() src 64 nsContentTreeOwner::ShowAsModal() src 65 nsWindowWatcher::OpenWindowInternal(nsIDOMWindow *,char const *,char const *,char const *,bool,bool,bool,nsITabParent *,nsIArray *,nsIDOMWindow * *) src 66 nsWindowWatcher::OpenWindow(nsIDOMWindow *,char const *,char const *,char const *,nsISupports *,nsIDOMWindow * *) src 67 NS_InvokeByIndex src 68 XPC_WN_CallMethod(JSContext *,unsigned int,JS::Value *) src 69 js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) src 70 Interpret src 71 js::RunScript(JSContext *,js::RunState &) src 72 js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) src 73 JS_CallFunctionValue(JSContext *,JS::Handle<JSObject *>,JS::Handle<JS::Value>,JS::HandleValueArray const &,JS::MutableHandle<JS::Value>) src 74 nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS *,unsigned short,XPTMethodDescriptor const *,nsXPTCMiniVariant *) src 75 nsXPCWrappedJS::CallMethod(unsigned short,XPTMethodDescriptor const *,nsXPTCMiniVariant *) src 76 PrepareAndDispatch src 77 SharedStub src 78 NS_InvokeByIndex src

https://crash-stats.mozilla.com/report/index/06d78bb8-b337-4fd9-88a5-a25f12150426 This looks pretty easy to fix: convert NPP_ClearSiteData to async and have the chrome process wait for an async response.

(In reply to Jim Mathies [:jimm] from comment #0) > If this is a deadlock caused by a sync win message plugin -> browser, why > didn't our deferral code pick it up and deal with it? I'm not sure what's > going on here. FWIW I think that we might have some bugs there. I'm investigating that as part of bug 1156800.

Sorry, this was on my todo list but I never got to it. Any time the chrome process sends a sync message to a plugin, we have a potential deadlock. Comment 1 is what we need to do.

This is turning out to be pretty common, and hopefully pretty straight forward to fix. Marking for triage.

Here's new variation of this: *********************************************************************************** 47e3996b-b057-4ee5-9f85-8a2612150427 Windows XP, 5.1.2600 Service Pack 3 NPSWF32_17_0_0_169.dll *********************************************************************************** PLUGIN ----------------------------------------------------------------------------------- 0 KiFastSystemCallRet 1 NtWaitForMultipleObjects 2 CreateFileMappingA 3 RealMsgWaitForMultipleObjectsEx 4 MsgWaitForMultipleObjects 5 mozilla::ipc::MessageChannel::WaitForSyncNotify() src 6 mozilla::ipc::MessageChannel::Send(IPC::Message *,IPC::Message *) src 7 mozilla::plugins::PPluginInstanceChild::SendShow(_NPRect const &,mozilla::plugins::SurfaceDescriptor const &,mozilla::plugins::SurfaceDescriptor *) src 8 mozilla::plugins::PluginInstanceChild::ShowPluginFrame() src 9 mozilla::plugins::PluginInstanceChild::InvalidateRectDelayed() src 10 MessageLoop::DoWork() src 11 base::MessagePumpForUI::DoRunLoop() src 12 base::MessagePumpWin::RunWithDispatcher(base::MessagePump::Delegate *,base::MessagePumpWin::Dispatcher *) src 13 base::MessagePumpWin::Run(base::MessagePump::Delegate *) src 14 MessageLoop::RunInternal() src 15 MessageLoop::RunHandler() src 16 MessageLoop::Run() src 17 XRE_InitChildProcess src 18 content_process_main(int,char * * const) src 19 wmain src 20 __tmainCRTStartup 21 BaseProcessStart CONTENT ----------------------------------------------------------------------------------- 0 KiFastSystemCallRet 1 ZwWaitForSingleObject 2 WaitForSingleObjectEx 3 WaitForSingleObject 4 PR_WaitCondVar src 5 mozilla::CondVar::Wait(unsigned int) src 6 mozilla::ipc::MessageChannel::WaitForSyncNotify() src 7 mozilla::ipc::MessageChannel::Send(IPC::Message *,IPC::Message *) src 8 mozilla::dom::PBrowserChild::SendGetTabOffset(mozilla::gfx::IntPointTyped<mozilla::LayoutDevicePixel> *) src 9 nsPluginFrame::GetRemoteTabChromeOffset() src 10 nsPluginFrame::GetWindowOriginInPixels(bool) src 11 nsPluginInstanceOwner::UpdateWindowPositionAndClipRect(bool) src 12 nsPluginFrame::DidSetWidgetGeometry() src 13 PluginDidSetGeometryEnumerator src 14 nsTHashtable<nsPtrHashKey<mozilla::DOMEventTargetHelper> >::s_EnumStub(PLDHashTable *,PLDHashEntryHdr *,unsigned int,void *) src 15 nsTHashtable<nsRefPtrHashKey<nsIContent> >::EnumerateEntries(PLDHashOperator (*)(nsRefPtrHashKey<nsIContent> *,void *),void *) src 16 nsRootPresContext::CollectPluginGeometryUpdates(mozilla::layers::LayerManager *) src 17 nsDisplayList::PaintRoot(nsDisplayListBuilder *,nsRenderingContext *,unsigned int) src 18 nsLayoutUtils::PaintFrame(nsRenderingContext *,nsIFrame *,nsRegion const &,unsigned int,unsigned int) src 19 PresShell::Paint(nsView *,nsRegion const &,unsigned int) src 20 nsViewManager::ProcessPendingUpdatesPaint(nsIWidget *) src 21 nsViewManager::ProcessPendingUpdatesForView(nsView *,bool) src 22 nsViewManager::ProcessPendingUpdates() src 23 nsRefreshDriver::Tick(__int64,mozilla::TimeStamp) src 24 mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver *,__int64,mozilla::TimeStamp) src 25 mozilla::RefreshDriverTimer::Tick(__int64,mozilla::TimeStamp) src 26 mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) src 27 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src 28 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) src 29 mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const &) src 30 mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const &) src 31 mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const &) src 32 mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const &) src 33 mozilla::ipc::MessageChannel::OnMaybeDequeueOne() src 34 MessageLoop::DoWork() src 35 mozilla::ipc::DoWorkRunnable::Run() src 36 nsThread::ProcessNextEvent(bool,bool *) src 37 NS_ProcessNextEvent(nsIThread *,bool) src 38 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) src 39 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate *) src 40 MessageLoop::RunHandler() src 41 MessageLoop::Run() src 42 nsBaseAppShell::Run() src 43 nsAppShell::Run() src 44 XRE_RunAppShell src 45 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate *) src 46 MessageLoop::RunHandler() src 47 MessageLoop::Run() src 48 XRE_InitChildProcess src 49 content_process_main(int,char * * const) src 50 wmain src 51 __tmainCRTStartup 52 BaseProcessStart BROWSER ----------------------------------------------------------------------------------- 0 google_breakpad::ExceptionHandler::WriteMinidump(std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > const &,bool (*)(wchar_t const *,wchar_t const *,void *,_EXCEPTION_POINTERS *,MDRawAssertionInfo *,bool),void *) src 1 CrashReporter::CreatePairedMinidumps(void *,unsigned long,nsIFile * *) src 2 mozilla::dom::CrashReporterParent::GeneratePairedMinidump<mozilla::plugins::PluginModuleChromeParent>(mozilla::plugins::PluginModuleChromeParent *) src 3 mozilla::plugins::PluginModuleChromeParent::TerminateChildProcess(MessageLoop *) src 4 mozilla::plugins::PluginModuleChromeParent::ShouldContinueFromReplyTimeout() src 5 mozilla::ipc::MessageChannel::ShouldContinueFromTimeout() src 6 mozilla::ipc::MessageChannel::Call(IPC::Message *,IPC::Message *) src 7 mozilla::plugins::PPluginModuleParent::CallNPP_ClearSiteData(nsCString const &,unsigned __int64 const &,unsigned __int64 const &,short *) src 8 mozilla::plugins::PluginModuleParent::NPP_ClearSiteData(char const *,unsigned __int64,unsigned __int64) src 9 nsPluginHost::ClearSiteData(nsIPluginTag *,nsACString_internal const &,unsigned __int64,__int64) src 10 NS_InvokeByIndex src 11 XPCWrappedNative::CallMethod(XPCCallContext &,XPCWrappedNative::CallMode) src 12 XPC_WN_CallMethod(JSContext *,unsigned int,JS::Value *) src 13 unknown: offset=unknown function=unknown 14 unknown: offset=unknown function=unknown 15 unknown: offset=unknown function=unknown 16 js::jit::MacroAssemblerX86::pushValue(js::jit::Address const &) src 17 EnterBaseline src 18 js::jit::EnterBaselineAtBranch(JSContext *,js::InterpreterFrame *,unsigned char *) src 19 Interpret src

Talked to Brad about maybe taking this on. The main entry point here is nsIPluginHost.clearSiteData. It's called from sanitize.js and ForgetAboutSite.jsm. I think the former one is the usual clear history dialog. I don't know what the latter one is for. Things are a bit trickier than I remembered because the call can throw, and the caller catches the exception. So just making it async won't really work. However, the nested event loop thing should be okay. The easiest place to put it would probably be here: http://mxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginModuleParent.cpp#2604 An even better alternative would be to rewrite the two JS callers to use callbacks or something. However, the sanitize.js code seems to be loaded and called from many different places, so you'd have to be careful about that.

FYI there is also a bug about dealing with NPP_ClearSiteData from a performance standpoint.

Yoric tells me that the JS bits are changing too.

Thanks Aaron. It looks like Yoric's changes in bug 1089695 will make this easier. It makes the sanitize.js code a generator so we can just clear asynchronously. The ForgetAboutSite.jsm use seems like it would be fine being async too. So we won't need a nested event loop.

Jim for the plugin changes, mconley for the browser front end try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=9800e751dd99

Comment on attachment 8621735 [details] [diff] [review] clear_site_data.patch Review of attachment 8621735 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/base/nsPluginHost.cpp @@ +1528,5 @@ > *aResult = nullptr; > return NS_ERROR_NOT_AVAILABLE; > } > > +void callClearSiteDataCallback(nsresult rv, void* closure) nit - Call vs. call. Same below - struct clearDataFromSitesClosure struct getSitesClosure void iterateMatchesAndClear void clearDataFromSites void getSites @@ +1587,5 @@ > + return; > + } > + tmp->callback->Callback(rv); > + tmp->callback->Release(); > + delete tmp; These manual deletes make me nervous. Why don't we hook these up to an auto pointer when we cast them so we know they get destroyed. Then we can stop tracking the deletes manually. @@ +1688,2 @@ > NS_ENSURE_SUCCESS(rv, rv); > + while (closure.keepWaiting) { Lets add a loud comment here bringing attention to this modal dispatch loop.

Comment on attachment 8621735 [details] [diff] [review] clear_site_data.patch I actually think that :mak might be a better reviewer for this, since he's already looking at bug 1089695.

I'm traveling from tomorrow and I'm not sure I can ensure you a good turn-around here... if you are in a hurry maybe you can try asking Yoric? What are the parts to be reviewed by a browser peer here, I assume everything but stuff in dom/plug-ins?

(In reply to Marco Bonardo [::mak] (Away June 17-22 - then ping me in Whistler) from comment #16) > I'm traveling from tomorrow and I'm not sure I can ensure you a good > turn-around here... if you are in a hurry maybe you can try asking Yoric? Okie doke, thanks. > What are the parts to be reviewed by a browser peer here, I assume > everything but stuff in dom/plug-ins? Correct!

Comment on attachment 8621735 [details] [diff] [review] clear_site_data.patch Review of attachment 8621735 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/sanitize.js @@ +235,5 @@ > : -1; > if (!this.range || age >= 0) { > let tags = ph.getPluginTags(); > for (let i = 0; i < tags.length; i++) { > yield Promise.resolve(); // Don't block the main thread too long I haven't seen that line of code on mozilla-central. On the other hand, I see it in one of my patches that hasn't landed yet because of a bug. Did you base your patch on mine?

(In reply to David Rajchenbach-Teller [:Yoric] (away June 22 - July 7th, use "needinfo") from comment #18) > Comment on attachment 8621735 [details] [diff] [review] > clear_site_data.patch > > Review of attachment 8621735 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/base/content/sanitize.js > @@ +235,5 @@ > > : -1; > > if (!this.range || age >= 0) { > > let tags = ph.getPluginTags(); > > for (let i = 0; i < tags.length; i++) { > > yield Promise.resolve(); // Don't block the main thread too long > > I haven't seen that line of code on mozilla-central. On the other hand, I > see it in one of my patches that hasn't landed yet because of a bug. Did you > base your patch on mine? I did, but then realized that your patch has test failures. I'm working on rebasing on trunk and fixing test failures of my own now.

In that case, I suggest you wait until I have landed bug 1089695, which makes sanitize asynchronous. That's much more complicated than it looks at first, because the code has... interesting behaviors. I am down to three failing tests and I hope to fix them before Whistler.

I have test passing locally with my patch now https://treeherder.mozilla.org/#/jobs?repo=try&revision=866526570f75

Comment on attachment 8621735 [details] [diff] [review] clear_site_data.patch Canceling review until I have the updated patch.

Comment on attachment 8625059 [details] [diff] [review] clearPluginCookies.patch Review of attachment 8625059 [details] [diff] [review]: ----------------------------------------------------------------- this review covers everything except the sanatize changes. ::: dom/plugins/base/nsPluginHost.cpp @@ +1530,5 @@ > } > > +#define ClearDataFromSitesClosure_CID {0x9fb21761, 0x2403, 0x41ad, {0x9e, 0xfd, 0x36, 0x7e, 0xc4, 0x4f, 0xa4, 0x5e}} > + > +class ClearDataFromSitesClosure : public nsISupports { comment header please explaining what this does. @@ +1564,5 @@ > + if (NS_FAILED(rv)) { > + closure->callback->Callback(rv); > + return; > + } > + if (closure->matches.Length() == 0) { nit - if (!val) @@ +1582,5 @@ > +} > + > +void ClearDataFromSites(InfallibleTArray<nsCString>& sites, nsCOMPtr<nsISupports> data) > +{ > + nsCOMPtr<ClearDataFromSitesClosure> closure = do_QueryInterface(data); Lets check the result or assert here on the result of this qi. @@ +1648,5 @@ > } > > +#define GetSitesClosure_CID {0x4c9268ac, 0x2fd1, 0x4f2a, {0x9a, 0x10, 0x7a, 0x09, 0xf1, 0xb7, 0x60, 0x3a}} > + > +class GetSitesClosure : public nsISupports { ditto - please comment me. @@ +1653,5 @@ > +public: > + NS_DECL_ISUPPORTS > + GetSitesClosure(const nsACString& domain, nsPluginHost* host) : mDomain(domain), mHost(host) { > + } > + nsCString mDomain; if you're going to use the 'm' prefix ina small helper class like this, please use it on all member variables and in both helper classes. @@ +1692,5 @@ > + return NS_OK; > +} > + > +void GetSites(InfallibleTArray<nsCString>& sites, nsCOMPtr<nsISupports> data) { > + // GetSitesClosure* closure = reinterpret_cast<GetSitesClosure*>(data->mVoidPtr); nit - junk? @@ +1696,5 @@ > + // GetSitesClosure* closure = reinterpret_cast<GetSitesClosure*>(data->mVoidPtr); > + MOZ_ASSERT(data, "got null data"); > + nsCOMPtr<GetSitesClosure> closure = do_QueryInterface(data); > + MOZ_ASSERT(!!closure, "couldn't QI to our closure"); > + //closure->sites.SwapElements(sites); ditto?

So, while updating the patch for Jim's review comments I decided that all these QI'ing to nsISupports and back were pretty ugly. This gets rid of all that.

Comment on attachment 8625059 [details] [diff] [review] clearPluginCookies.patch Review of attachment 8625059 [details] [diff] [review]: ----------------------------------------------------------------- Sorry I was reviewing the patch and I reviewed the previous version... btw, I think not much changed in the parts I looked at, so we should still be fine. I'm sure with Yoric's patch this will be much cleaner, but let's assume this will land first and then Yoric can just update his patch to make this more streamlined. ::: browser/base/content/sanitize.js @@ +240,5 @@ > + this.clearPluginCookies( > + function() { > + TelemetryStopwatch.finish("FX_SANITIZE_PLUGINS"); > + TelemetryStopwatch.finish("FX_SANITIZE_COOKIES"); > + aCallback(true); what's the point of passing true? we don't seem to use this info. @@ +260,3 @@ > if (!this.range || age >= 0) { > let tags = ph.getPluginTags(); > + function iterate(tags, index) { I feel like this would be much easier implemented with promises and a simple for...of loop, you can return Promise.all() ::: dom/plugins/base/nsIPluginHost.idl @@ +27,5 @@ > boolean checkWhitelist(in AUTF8String pageURI, in AUTF8String objectURI); > }; > > +[scriptable, uuid(9c311778-7c2c-4ad8-b439-b8a2786a20dd)] > +interface nsIClearSiteDataCallback : nsISupports could you please add the function decorator to this, so we don't have to explicitly build an object for it? and then remove the now useless objects. ::: toolkit/forgetaboutsite/ForgetAboutSite.jsm @@ +44,5 @@ > const Ci = Components.interfaces; > const Cu = Components.utils; > > this.ForgetAboutSite = { > + removeDataFromDomain: function CRH_removeDataFromDomain(aDomain, aCallback) Is there any valid reason to not make this just return a promise and use a Task in the tests? It would allow for cleaner code, and unhandled rejections would be reported in a useful way. The code would not change much. you could collect promises from the plugins code callbacks and return Promise.all([promises]) at the end. The test could just yield in a Task.

Comment on attachment 8625880 [details] [diff] [review] clearPluginCookies.patch Review of attachment 8625880 [details] [diff] [review]: ----------------------------------------------------------------- done already (<3 time machine)

Comment on attachment 8625880 [details] [diff] [review] clearPluginCookies.patch Review of attachment 8625880 [details] [diff] [review]: ----------------------------------------------------------------- Much cleaner, thanks.

Comment on attachment 8626370 [details] [diff] [review] clearPluginCookies.patch Review of attachment 8626370 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/sanitize.js @@ +259,4 @@ > if (!this.range || age >= 0) { > let tags = ph.getPluginTags(); > + function iterate(tag) { > + let deferred = Promise.defer(); Promise.defer is deprecated in new code, and slowly being removed from old code. We use either PromiseUtils.defer() or, even better, new Promise(): let promises = []; for (let tag of tags) { let promise = new Promise(resolve => { ... ...resolve(); ... }); promises.push(promise); } @@ +267,5 @@ > + ph.clearSiteData(tag, null, FLAG_CLEAR_ALL, -1, function() { > + deferred.resolve(); > + }); > + } else { > + deferred.resolve(); can more resolve() out if the if/else since it's called regardless. @@ +272,4 @@ > } > + }; > + ph.clearSiteData(tag, null, FLAG_CLEAR_ALL, age, onClear); > + } catch (ex) { Should we reportError this exception, would that help figuring mistakes? If it can happen in common cases, and thus is uninteresting, nevermind. ::: toolkit/forgetaboutsite/ForgetAboutSite.jsm @@ +12,5 @@ > "resource://gre/modules/PlacesUtils.jsm"); > XPCOMUtils.defineLazyModuleGetter(this, "Downloads", > "resource://gre/modules/Downloads.jsm"); > +XPCOMUtils.defineLazyModuleGetter(this, "Promise", > + "resource://gre/modules/Promise.jsm"); let's just use DOM promises, you don't need this. @@ +96,3 @@ > for (let i = 0; i < tags.length; i++) { > + let deferred = Promise.defer(); > + promises.push(deferred.promise); as above, please use new Promise() @@ +176,5 @@ > // domain, so just trash it all > let np = Cc["@mozilla.org/network/predictor;1"]. > getService(Ci.nsINetworkPredictor); > np.reset(); > + return Promise.all(promises); So, removeDataFromDomain is a mixup of sync and async, waiting for something but not something else, at the moment. The best thing to do here would be to convert it to a Task.async, like removeDataFromDomain: Task.async(function* (aDomain) { ... }) then we could remove the Downloads Task.spawn, since we'd already be in a task, and just yield on the promises you will create here (yield new Promise()...). Also onContentPrefsRemovalFinished should likely be wrapped in a promise :( Though, this might also be done in a follow-up, I just pointed it out, since you are returning a promise, and that way would make the code more maintainable. If you don't do that, please file a bug and I'll be happy to mentor it.

(In reply to Marco Bonardo [::mak] from comment #33) > @@ +267,5 @@ > > + ph.clearSiteData(tag, null, FLAG_CLEAR_ALL, -1, function() { > > + deferred.resolve(); > > + }); > > + } else { > > + deferred.resolve(); > > can more resolve() out if the if/else since it's called regardless. Sigh, stupid typos: "Can move resolve() out of the if/else, since it's called regardless."

(In reply to Marco Bonardo [::mak] from comment #34) > (In reply to Marco Bonardo [::mak] from comment #33) > > @@ +267,5 @@ > > > + ph.clearSiteData(tag, null, FLAG_CLEAR_ALL, -1, function() { > > > + deferred.resolve(); > > > + }); > > > + } else { > > > + deferred.resolve(); > > > > can more resolve() out if the if/else since it's called regardless. > > Sigh, stupid typos: > "Can move resolve() out of the if/else, since it's called regardless." In the if block resolve() is called from a callback, so I don't think it can be removed from the if/else.

(In reply to Marco Bonardo [::mak] from comment #33) > Comment on attachment 8626370 [details] [diff] [review] > clearPluginCookies.patch > > Review of attachment 8626370 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +272,4 @@ > > } > > + }; > > + ph.clearSiteData(tag, null, FLAG_CLEAR_ALL, age, onClear); > > + } catch (ex) { > > Should we reportError this exception, would that help figuring mistakes? > If it can happen in common cases, and thus is uninteresting, nevermind. This happens in common cases (such as "the plugin is not loaded and isn't flash") > @@ +176,5 @@ > > // domain, so just trash it all > > let np = Cc["@mozilla.org/network/predictor;1"]. > > getService(Ci.nsINetworkPredictor); > > np.reset(); > > + return Promise.all(promises); > > So, removeDataFromDomain is a mixup of sync and async, waiting for something > but not something else, at the moment. The best thing to do here would be to > convert it to a Task.async, like > removeDataFromDomain: Task.async(function* (aDomain) { > ... > }) > then we could remove the Downloads Task.spawn, since we'd already be in a > task, and just yield on the promises you will create here (yield new > Promise()...). Also onContentPrefsRemovalFinished should likely be wrapped > in a promise :( > Though, this might also be done in a follow-up, I just pointed it out, since > you are returning a promise, and that way would make the code more > maintainable. If you don't do that, please file a bug and I'll be happy to > mentor it. I assume this is what's going on in Yorik's bug, so not going to do this now.

Hey Brad, does this need a uuid change ? remote: Push rejected because the following IDL interfaces were remote: modified without changing the UUID: remote: - nsIPluginHost in changeset 5ace0ac37af9 also do you have a try link handy ?

Backed out for win32 build bustage. Win64 builds were fine. https://treeherder.mozilla.org/logviewer.html#?job_id=11256431&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=11258607&repo=mozilla-inbound (post follow-up) https://hg.mozilla.org/integration/mozilla-inbound/rev/63c84b5971c4

Also OSX mochitest asserts (10.6 and 10.10). https://treeherder.mozilla.org/logviewer.html#?job_id=11258850&repo=mozilla-inbound

Same asserts on Linux.

Had to revert this again for the asserts: https://treeherder.mozilla.org/logviewer.html#?job_id=11276293&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/b7d55457e9ce

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #46) > Created attachment 8630441 [details] [diff] [review] > clear_plugin_data.patch What changed to fix the xp issue?

The assert was: ###!!! ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr', file c:\builds\moz2_slave\m-in-w32-d-0000000000000000000\build\src\obj-firefox\dist\include\nsCOMPtr.h, line 401

This bug's patch was missing some 'override' annotations on new methods ("Callback", "SitesWithData", "RecvReturnClearSiteData", and "RecvReturnSitesWithData"). That causes clang 3.6 & newer to spam a "-Winconsistent-missing-override" build warning. (which busts --enable-warnings-as-errors builds with these compilers) I'll push a followup shortly to add that keyword, with blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2.

Pushed that followup: https://hg.mozilla.org/integration/mozilla-inbound/rev/68f9ee587e26

https://hg.mozilla.org/mozilla-central/rev/f3a229a8202e https://hg.mozilla.org/mozilla-central/rev/68f9ee587e26