Focus controller holds previously focused window alive

RESOLVED WONTFIX

Status

()

defect
RESOLVED WONTFIX
11 years ago
5 months ago

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

Tracking

Trunk
Points:
---
Bug Flags:
blocking1.9.1 -
wanted1.9.1 +

Firefox Tracking Flags

(status1.9.1 wontfix)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Posted patch Patch (obsolete) — Splinter Review
We can get away with weak references for the *previous* window/element that were focused.
Attachment #375097 - Flags: review?(enndeakin)
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.
Or at least Bug 178324 will remove those member variables.
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.
Attachment #375097 - Attachment is obsolete: true
Attachment #375097 - Flags: review?(enndeakin)
Oh, and yeah, Neal says that this whole thing is going away for 1.9.2.
Posted patch Patch (obsolete) — Splinter Review
smaug, what do you think?
Attachment #375241 - Flags: review?(Olli.Pettay)
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)
Posted patch Patch, v2Splinter Review
With those changes
Attachment #375241 - Attachment is obsolete: true
Attachment #375267 - Flags: review?(Olli.Pettay)
Attachment #375241 - Flags: review?(Olli.Pettay)
Attachment #375267 - Flags: superreview+
Attachment #375267 - Flags: review?(Olli.Pettay)
Attachment #375267 - Flags: review+
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
Not blocking on this, but I'll approve the patch as this will help with leaks and leak hunting in general.
Flags: wanted1.9.1?
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Attachment #375267 - Flags: approval1.9.1+
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.
Keywords: checkin-needed
Leaks appear to have persisted even after backing this patch out so I think we should try landing again.
Keywords: checkin-needed
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.
Hrm, ok. I'll see if I can reproduce.
Attachment #375267 - Flags: approval1.9.1+ → approval1.9.1-
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
Flags: wanted1.9.1.x?
Ben: Any more work here? Did this code actually go away with 1.9.2 or do we need a fix there as well?
status1.9.1: --- → ?
Flags: wanted1.9.1.x?
Yeah, we don't need this for 1.9.2. Not sure about 1.9.1.
Let's just close this.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.