315708 - Should release found link and current window object from nsTypeAheadFind.cpp

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

In nsTypeAheadFind, we add refs to link or window object when finding text or link. But these objects aren't release until next finding.
Currently, nsTypeAheadFind can know when these objects are not needed. The time is running |SetDocShell|. It's called when changing tab or unloaded the document in current tab. In this time, we should release these object.

This is regression of pseudo-focus behavior implementation.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The second half of the patch makes sense, but why the first half, if they're about to be released anyway?

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> but why the first half, if they're
> about to be released anyway?

I don't need to release these on destructor?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsCOMPtr's destructor releases the object it points to.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 202379 [details] [diff] [review]
Patch rv1.0

thanks, I'll attach new patch.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

michaell+bmo:

This bug depends on bug 314288 that is calling nsTypeAheadFind::SetDocShell on hidden the document. But that bug is already denied to check-in to 1.8 branch...

Comment 9

[Michael Lefevre]

Thanks for explaining. Sorry if I'm just adding to noise, but given that the reason for not taking main patch for bug 314288 on branch was because it was too close to release, I wonder if approval for the other patch would be reconsidered on the different criteria for 1.8.1? I'm afraid I don't know enough to know if that's a reasonable question for this patch. dbaron? mconnor?

Comment 10

[:Gavin Sharp [email: [e-mail]]]

The fact that a patch was denied approval for the 1.8 branch pre-1.5 doesn't mean that it can't land on the 1.8 branch for 1.8.1 (Firefox 2). It would of course help if the regressions are well-known and fixed, if applicable.

Comment 11

[Mike Connor [:mconnor]]

We didn't take that patch for 1.8 final because it was extremely late in the game.  That doesn't mean we won't take it for 1.8.1, and I would say we absolutely need/want to take both, assuming the regression gets fixed (bug 314819)

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in to 1.8 branch.

Comment 13

[Daniel Veditz [:dveditz]]

You need to work through the regression chain before we can consider for a stability release.