490715 - Focus controller holds previously focused window alive

Status: RESOLVED WONTFIX

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

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch

We can get away with weak references for the *previous* window/element that were focused.

Comment 1

[Olli Pettay [:smaug]]

Would it work if nsFocusController unlinked those nsCOMPtr members?

Is this something for 1.9.1? Because I hope the whole class is going away soon
on trunk.

Comment 2

[Olli Pettay [:smaug]]

Or at least Bug 178324 will remove those member variables.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 375097 [details] [diff] [review]
Patch

So unlinking doesn't work because the focus controller belongs to a live window and won't actually be unlinked.

I'll look into doing this without weak refs since they are apparently expensive for elements.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Oh, and yeah, Neal says that this whole thing is going away for 1.9.2.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch

smaug, what do you think?

Comment 6

[Olli Pettay [:smaug]]

Comment on attachment 375241 [details] [diff] [review]
Patch

>+NS_IMETHODIMP
>+nsFocusController::ForgetDyingWindow(nsIDOMWindowInternal* aWindow)
>+{
>+  nsCOMPtr<nsPIDOMWindow> win(do_QueryInterface(aWindow));
If you change the parameter type, no need to QI nor use nsCOMPtr.
I know, nsIFocusController uses nsIDOM***, but it actually shouldn't.

>+  if (!(win && (win = win->GetOuterWindow()))) {
>+    return NS_OK;
>+  }
>+
>+  if (win == mCurrentWindow) {
>+    mCurrentWindow = nsnull;
>+    mCurrentElement = nsnull;
>+  }
>+
>+  if (win == mPreviousWindow) {
>+    mPreviousWindow = nsnull;
>+    mPreviousElement = nsnull;
>+  }

I wonder if it is guaranteed that mXXXElement is always from 
mXXXWindow. Could you perhaps do
if (!mXXXElement->GetOwnerDoc() ||
    !mXXXElement->GetOwnerDoc()->GetWindow() ||
    mXXXElement->GetOwnerDoc()->GetWindow() == win) {
  mXXXElement = nsnull;
}
I just want to be *very* careful when dealing with focus handling code in
branch. Fortunately this code is going away soon.


>+  NS_IMETHOD ForgetDyingWindow(nsIDOMWindowInternal* aWindow) = 0;
I'd call this just Disconnect(nsPIDOMWindow* aWindow)

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2

With those changes

Comment 8

[Johnny Stenback (:jst)]

Not blocking on this, but I'll approve the patch as this will help with leaks and leak hunting in general.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I checked this in, but had to back it out due to test failures:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1241753156.1241760168.29896.gz#err0

NEXT ERROR *** 707 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | Error thrown during test: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMNSDocument.hasFocus]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: http://localhost:8888/tests/SimpleTest/TestRunner.js :: anonymous :: line 75"  data: no] - got 0, expected 1

This bug seemed the most likely culprit.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Leaks appear to have persisted even after backing this patch out so I think we should try landing again.

Comment 11

[David Baron :dbaron:]

No, this backout definitely did fix the errors with the message in comment 9, which happened on every unit test build from when this was landed until it was backed out.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Hrm, ok. I'll see if I can reproduce.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 375267 [details] [diff] [review]
Patch, v2

Revoking approval. We're cutting back on potential churn here. We can try again for 3.5.1

Comment 14

[Samuel Sidler (old account; do not CC)]

Ben: Any more work here? Did this code actually go away with 1.9.2 or do we need a fix there as well?

Comment 15

[Neil Deakin]

Yeah, we don't need this for 1.9.2. Not sure about 1.9.1.

Comment 16

[Neil Deakin]

Let's just close this.