885171 - Cycle-collect and wrappercache Navigator

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We'll need this for bug 838146 and it seems good to land this separately.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Make Navigator wrappercached and cycle-collected, and have it hold a strong reference to its window always.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I thought I reviewed something like this in some bug..

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 765172 [details] [diff] [review]
Make Navigator wrappercached and cycle-collected, and have it hold a strong reference to its window always.

Better to null check mWindow usage. Otherwise if something happens to call any of the methods using mWindow during unlink or dtor, we'll crash.


> Navigator::GetUserAgent(nsAString& aUserAgent)
> {
>   nsresult rv = NS_GetNavigatorUserAgent(aUserAgent);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  nsCOMPtr<nsPIDOMWindow> win(do_QueryReferent(mWindow));
>-  if (!win || !win->GetDocShell()) {
>+  if (!mWindow->GetDocShell()) {
!mWindow || !mWindow->GetDocShell()

> Navigator::GetMimeTypes(nsISupports** aMimeTypes)
> {
>-  nsCOMPtr<nsPIDOMWindow> win(do_QueryReferent(mWindow));
>-  MOZ_ASSERT(win);
ugh.

NS_ENSURE_STATE(mWindow)


> NS_IMETHODIMP
> Navigator::GetPlugins(nsISupports** aPlugins)
> {
>-  nsCOMPtr<nsPIDOMWindow> win(do_QueryReferent(mWindow));
>-  MOZ_ASSERT(win);
>-

ditto


>-  nsCOMPtr<nsPIDOMWindow> win(do_QueryReferent(mWindow));
>-
>-  if (!win || !win->GetDocShell()) {
>+  if (!mWindow->GetDocShell()) {
!mWindow || mWindow->GetDocShell()

> Navigator::JavaEnabled(bool* aReturn)
> {
>   Telemetry::AutoTimer<Telemetry::CHECK_JAVA_ENABLED> telemetryTimer;
>   // Return true if we have a handler for "application/x-java-vm",
>   // otherwise return false.
>   *aReturn = false;
> 
>-  nsCOMPtr<nsPIDOMWindow> win(do_QueryReferent(mWindow));
>-  MOZ_ASSERT(win);
>-

NS_ENSURE_STATE(mWindow)

>   NS_ENSURE_ARG_POINTER(aIdleObserver);
> 
>-  nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow);
>-  NS_ENSURE_TRUE(win, NS_ERROR_UNEXPECTED);

NS_ENSURE_STATE(mWindow)

> Navigator::RemoveIdleObserver(nsIIdleObserver* aIdleObserver)
> {
>   if (!nsContentUtils::IsIdleObserverAPIEnabled()) {
>     NS_WARNING("The IdleObserver API has been disabled");
>     return NS_OK;
>   }
> 
>   NS_ENSURE_ARG_POINTER(aIdleObserver);
> 
>-  nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow);
>-  NS_ENSURE_TRUE(win, NS_ERROR_UNEXPECTED);
ditto


> Navigator::Vibrate(const JS::Value& aPattern, JSContext* cx)
> {
>-  nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow);
>-  NS_ENSURE_TRUE(win, NS_OK);

and here

> Navigator::RegisterContentHandler(const nsAString& aMIMEType,
>                                   const nsAString& aURI,
>                                   const nsAString& aTitle)
> {
>-  nsCOMPtr<nsPIDOMWindow> win(do_QueryReferent(mWindow));
>-
>-  if (!win || !win->GetOuterWindow() || !win->GetDocShell()) {
>+  if (!mWindow->GetOuterWindow() || !mWindow->GetDocShell()) {
null check

...and more

>-  nsPIDOMWindow *GetWindow();
>+  nsPIDOMWindow *Window()
>+  {
>+    return mWindow;
>+  }

This is wrong naming. Window() may return null. Let's try to keep the non-Get* naming for stuff that will *never* return null.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Make Navigator wrappercached and cycle-collected, and have it hold a strong reference to its window always.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 765461 [details] [diff] [review]
Make Navigator wrappercached and cycle-collected, and have it hold a strong reference to its window always.

In Navigator::CheckPermission better to return false if there is no
mWindow.

Not sure why you changed nsNavigatorSH::PreCreate. I'd keep the
null check.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d265226f218c

Comment 7

[Boris Zbarsky [:bzbarsky]]

I did make the changes from comment 5 before pushing.