If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Cycle-collect and wrappercache Navigator

RESOLVED FIXED in mozilla24

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

unspecified
mozilla24
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

We'll need this for bug 838146 and it seems good to land this separately.
Created attachment 765172 [details] [diff] [review]
Make Navigator wrappercached and cycle-collected, and have it hold a strong reference to its window always.
Attachment #765172 - Flags: review?(bugs)
I thought I reviewed something like this in some bug..
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.
Attachment #765172 - Flags: review?(bugs) → review-
Created attachment 765461 [details] [diff] [review]
Make Navigator wrappercached and cycle-collected, and have it hold a strong reference to its window always.
Attachment #765461 - Flags: review?(bugs)
Attachment #765172 - Attachment is obsolete: true
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.
Attachment #765461 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/d265226f218c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [need review]
Target Milestone: --- → mozilla24
I did make the changes from comment 5 before pushing.
Flags: in-testsuite-
Depends on: 886213
You need to log in before you can comment on or make changes to this bug.