Closed
Bug 490715
Opened 12 years ago
Closed 11 years ago
Focus controller holds previously focused window alive
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
WONTFIX
Tracking | Status | |
---|---|---|
status1.9.1 | --- | wontfix |
People
(Reporter: bent.mozilla, Assigned: bent.mozilla)
Details
Attachments
(1 file, 2 obsolete files)
4.94 KB,
patch
|
smaug
:
review+
smaug
:
superreview+
beltzner
:
approval1.9.1-
|
Details | Diff | Splinter Review |
We can get away with weak references for the *previous* window/element that were focused.
Attachment #375097 -
Flags: review?(enndeakin)
Comment 1•12 years ago
|
||
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•12 years ago
|
||
Or at least Bug 178324 will remove those member variables.
Assignee | ||
Comment 3•12 years ago
|
||
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)
Assignee | ||
Comment 4•12 years ago
|
||
Oh, and yeah, Neal says that this whole thing is going away for 1.9.2.
Assignee | ||
Comment 5•12 years ago
|
||
smaug, what do you think?
Attachment #375241 -
Flags: review?(Olli.Pettay)
Comment 6•12 years ago
|
||
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)
Assignee | ||
Comment 7•12 years ago
|
||
With those changes
Attachment #375241 -
Attachment is obsolete: true
Attachment #375267 -
Flags: review?(Olli.Pettay)
Attachment #375241 -
Flags: review?(Olli.Pettay)
Updated•12 years ago
|
Attachment #375267 -
Flags: superreview+
Attachment #375267 -
Flags: review?(Olli.Pettay)
Attachment #375267 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
Comment 8•12 years ago
|
||
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-
Updated•12 years ago
|
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
Assignee | ||
Comment 10•12 years ago
|
||
Leaks appear to have persisted even after backing this patch out so I think we should try landing again.
Keywords: checkin-needed
Comment 11•12 years ago
|
||
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.
Updated•12 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 12•12 years ago
|
||
Hrm, ok. I'll see if I can reproduce.
Updated•12 years ago
|
Attachment #375267 -
Flags: approval1.9.1+ → approval1.9.1-
Comment 13•12 years ago
|
||
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
Updated•12 years ago
|
Flags: wanted1.9.1.x?
Comment 14•12 years ago
|
||
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?
Comment 15•12 years ago
|
||
Yeah, we don't need this for 1.9.2. Not sure about 1.9.1.
Comment 16•11 years ago
|
||
Let's just close this.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
Updated•11 years ago
|
Updated•2 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•