Closed Bug 1384588 Opened 7 years ago Closed 7 years ago

The observer for net:current-toplevel-outer-content-windowid is needlessly weak

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1366822

People

(Reporter: ehsan.akhgari, Unassigned)

References

(Blocks 1 open bug)

Details

See https://searchfox.org/mozilla-central/rev/8a61c71153a79cda2e1ae7d477564347c607cc5f/netwerk/protocol/http/nsHttpHandler.cpp#526. I'm not sure why that is, is there a good reason Kershaw? The QI() for the weak reference shows up in profiles when pages call .focus() on an element under a call stack like this: nsWeakReference::QueryReferent(nsID const&, void**) nsQueryReferent::operator()(nsID const&, void**) const nsCOMPtr_base::assign_from_helper(nsCOMPtr_helper const&, nsID const&) nsObserverList::FillObserverArray(nsCOMArray<nsIObserver>&) nsObserverList::NotifyObservers(nsISupports*, char const*, char16_t const*) nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*) nsObserverService::NotifyObservers net:current-toplevel-outer-content-windowid NS_NotifyCurrentTopLevelOuterContentWindowId(unsigned long) nsFocusManager::Focus(nsPIDOMWindowOuter*, nsIContent*, unsigned int, bool, bool, bool, bool, nsIContent*) nsFocusManager::SetFocusInner(nsIContent*, int, bool, bool) nsFocusManager::SetFocus(nsIDOMElement*, unsigned int) mozilla::dom::Element::Focus(mozilla::ErrorResult&) mozilla::dom::HTMLElementBinding::focus(JSContext*, JS::Handle<JSObject*>, nsGenericHTMLElement*, JSJitMethodCallArgs const&) mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) js::Wrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) js::proxy_Call(JSContext*, unsigned int, JS::Value*) js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) Interpret(JSContext*, js::RunState&) js::RunScript(JSContext*, js::RunState&) prepare/< js::RunScript js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) 0x38ea1c81e106 0x7f16ffaff1ff SimplePromise.prototype.resolve 0x38ea1c88f710 0x7f17000cf4cf SimplePromise.prototype.then 0x38ea1c8128a9 EnterBaseline(JSContext*, js::jit::EnterJitData&) js::jit::EnterBaselineMethod(JSContext*, js::RunState&) Interpret(JSContext*, js::RunState&) js::RunScript(JSContext*, js::RunState&) prepare BenchmarkState.prototype.prepareCurrentSuite/frame.onload js::RunScript js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) EventListenerManager::HandleEventInternal load mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) EventDispatcher::Dispatch nsGlobalWindow::PostHandleEvent(mozilla::EventChainPostVisitor&) mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) EventDispatcher::Dispatch nsDocumentViewer::LoadComplete(nsresult) nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) nsDocLoader::DocLoaderIsEmpty(bool) nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) nsDocument::UnblockOnload(bool) nsBindingManager::DoProcessAttachedQueue() mozilla::detail::RunnableMethodImpl<nsBindingManager*, void (nsBindingManager::*)(), true, (mozilla::RunnableKind)0>::Run() mozilla::SchedulerGroup::Runnable::Run() nsThread::ProcessNextEvent(bool, bool*) NS_ProcessNextEvent(nsIThread*, bool) mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) MessageLoop::Run() nsBaseAppShell::Run() XRE_RunAppShell() MessageLoop::Run() XRE_InitChildProcess(int, char**, XREChildData const*) XRE_InitChildProcess (root) Since there are other observers registered here which are actively being managed it seems we can also actively unregister this one when we're about to away and not pay this QI cost needlessly?
Flags: needinfo?(kechang)
NS_NotifyCurrentTopLevelOuterContentWindowId() and its caller in nsFocusManager are going to be removed in bug 1366822 [1], so the observer code in nsHttpHandler [2] will not be called every time when an element is focused. FWIW, "net:current-toplevel-outer-content-windowid" is used for notifying necko the current top-level content window id. In bug 1366822, we want to use front end code to send the notification to necko. After bug 1366822 is landed, the code in [2] will be also happened only in parent process. So, the performance issue in this bug will be gone soon. I think we can close this bug. What do you think, Ehsan? [1] https://bug1366822.bmoattachments.org/attachment.cgi?id=8884205 [2] http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/netwerk/protocol/http/nsHttpHandler.cpp#2372
Flags: needinfo?(kechang) → needinfo?(ehsan)
(In reply to Kershaw Chang [:kershaw] from comment #1) > NS_NotifyCurrentTopLevelOuterContentWindowId() and its caller in > nsFocusManager are going to be removed in bug 1366822 [1], so the observer > code in nsHttpHandler [2] will not be called every time when an element is > focused. Great! > FWIW, "net:current-toplevel-outer-content-windowid" is used for notifying > necko the current top-level content window id. > In bug 1366822, we want to use front end code to send the notification to > necko. After bug 1366822 is landed, the code in [2] will be also happened > only in parent process. > > So, the performance issue in this bug will be gone soon. I think we can > close this bug. What do you think, Ehsan? Yes, indeed bug 1366822 will take care of the issue here. If this indeed needs to stay a weak reference after that, please feel free to close this bug. Thanks for the quick response!
Flags: needinfo?(ehsan)
Dup to bug 1366822 based on comment #2.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.