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

RESOLVED DUPLICATE of bug 1366822

Status

()

defect
RESOLVED DUPLICATE of bug 1366822
2 years ago
2 years ago

People

(Reporter: Ehsan, Unassigned)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Reporter

Description

2 years ago
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)
Reporter

Updated

2 years ago
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)
Reporter

Comment 2

2 years ago
(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: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1366822
You need to log in before you can comment on or make changes to this bug.