Closed Bug 490715 Opened 12 years ago Closed 11 years ago

Focus controller holds previously focused window alive

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
Tracking Status
status1.9.1 --- wontfix

People

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

Details

Attachments

(1 file, 2 obsolete files)

Attached 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.
Attached 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)
Attached 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.
Keywords: checkin-needed
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: 11 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.