Closed Bug 314288 Opened 19 years ago Closed 19 years ago

FastFind is broken after find in this page in frame page

Categories

(Toolkit :: Find Toolbar, defect, P1)

Product:
Toolkit
Component:
Find Toolbar
Version:
1.8.0 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.8.1beta2

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: fixed1.8.1, regression)

Attachments

(2 files, 1 obsolete file)

Patch rv2.0
2.50 KB, patch
mconnor
: review+
bzbarsky
: superreview+
asa
: approval1.8rc2-
Details | Diff | Splinter Review
branch rollup
3.39 KB, patch
beltzner
: approval1.8.1+
Details | Diff | Splinter Review 
1. Show frame page (e.g., http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2943&action=view )
2. Find "Shift_JIS" with Ctrl + F
3. Go to another page using bookmark
4. Find any text

In step 4, the FastFind is broken. I cannot find the text.
If in step 2, we cannot reproduce this bug.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051028 Firefox/1.6a1
And if I dont' do step 2, I cannot reproduce in step 4.
But back to frame page, I cannot find text in frame page.
Severity: normal → major
I can reproduce with 1.8 branch!
This is serious!
Version: Trunk → 1.5 Branch
Flags: blocking1.8rc1?
Confirmed on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051025 Firefox/1.5 ID:2005102519 using a form of the instructions in comment 0.
This is regression of bug 298293 that is to enable bfcache.
I set "browser.sessionhistory.max_total_view" to "0", I cannot reproduce this bug.
Depends on: 298293
Summary: FastFind is broken after find in this page in frame page(?) → FastFind is broken after find in this page in frame page
Attached patch Patch rv1.0 (obsolete) — — Splinter Review 
This patch fixes this bug.

In nsTypeAheadFind, we use tricky way for get current document.
http://lxr.mozilla.org/mozilla/source/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp#1156
1155 already_AddRefed<nsIPresShell>
1156 nsTypeAheadFind::GetPresShell()
1157 {
1158   if (!mPresShell)
1159     return nsnull;
1160 
1161   nsIPresShell *shell = nsnull;
1162   CallQueryReferent(mPresShell.get(), &shell);
1163   if (shell) {
1164     nsPresContext *pc = shell->GetPresContext();
1165     if (!pc || !nsCOMPtr<nsISupports>(pc->GetContainer())) {
1166       NS_RELEASE(shell);
1167     }
1168   }
1169 
1170   return shell;
1171 }
1172 

The case of without bfcache, the unloaded page's nsPresContext is gone, therefore, we release the shell(#166).
But, with bfcache, the bfcache may refer the nsPresContext. Therefore, we cannot release it. So, we can always get previous page's nsPresShell in nsTypeAheadFind::Find.

Simply, we need to reset the mPresShell by setDocShell method when the page is undloaded.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #201233 - Flags: review?(mconnor)
Attachment #201233 - Flags: approval1.8rc1?
Priority: -- → P1
Target Milestone: --- → Firefox1.5rc1
I'll be away 5 or 6 hours. Sorry.
Comment on attachment 201233 [details] [diff] [review]
Patch rv1.0

hrm, shouldn't it be done on-page-show?
Attachment #201233 - Flags: review?(mconnor)
Attachment #201233 - Flags: review-
Attachment #201233 - Flags: approval1.8rc1?
Attached patch Patch rv2.0 — — Splinter Review 
Hm... We should not run |setDocShell| if the browser is not current tab.
I think that this patch is bettter.
Attachment #201233 - Attachment is obsolete: true
Attachment #201254 - Flags: review?(mconnor)
Attachment #201254 - Flags: approval1.8rc1?
Does XPFE need similar changes?

That said, I do NOT understand comment 7.  With bfcache, there will be a prescontext, but it'll have no container, so you should still be releasing the shell.  Why is that not happening?
I confirmed with debug mode of VC++ on windows.
If bfcache is enabled, pc->GetContainer() returns not null in step 4.
Is this bfcache's bug?
Boris:

In comment 7:
> The case of without bfcache, the unloaded page's nsPresContext is gone,
> therefore, we release the shell(#166).
> But, with bfcache, the bfcache may refer the nsPresContext.

This is wrong, sorry. nsPresContext is not gone.
But |nsPresContext::GetContainer| should return null in this case, but it's not so.
> Is this bfcache's bug?

Could be.  Depends on when you're calling this code...

Is there a container on the document for that presshell?
(In reply to comment #10)
> Created an attachment (id=201254) [edit]
> Patch rv2.0
> 
> Hm... We should not run |setDocShell| if the browser is not current tab.
> I think that this patch is bettter.
> 

this looks good to take before RC2.
(In reply to comment #14)
> > Is this bfcache's bug?
> 
> Could be.  Depends on when you're calling this code...
> 
> Is there a container on the document for that presshell?

I cannot understand "container", sorry.
But we call this function when the user type the find text into Find Toolbar. 
Therefore, there is not the document. The document is unloaded already.


> I cannot understand "container", sorry.

http://lxr.mozilla.org/seamonkey/source/content/base/public/nsIDocument.h#581

> Therefore, there is not the document.

The document is in bfcache, I would assume.  Note that the presshell holds an owning reference to the document, so if you have a live presshell you have a document.
mPresShell and mDocShell are always set with do_GetWeakReference in nsTypeAheadFind.cpp...
Um.  The code cited in comment 7 gets an nsIPresShell from the weak reference.  At that point, you can look at shell->GetDocument()->GetDocumentContainer().  Is that null?
(In reply to comment #19)
> Um.  The code cited in comment 7 gets an nsIPresShell from the weak reference. 
> At that point, you can look at shell->GetDocument()->GetDocumentContainer(). 
> Is that null?
> 

shell->GetDocument()->GetContainer() ?
Er, yes.  I wish the accessor and the member names matched....
shell->GetDocument()->GetContainer() is not null.
(In reply to comment #11)
>Does XPFE need similar changes?
XPFE doesn't try to do anything tricky with fast find.
OK.  What's the documentURI of shell->GetDocument() ?  Is it the URI of the page that's in bfcache, or of the newly-loaded page?
(In reply to comment #24)
> OK.  What's the documentURI of shell->GetDocument() ?  Is it the URI of the
> page that's in bfcache, or of the newly-loaded page?
> 

the URI is old page that is in bfcache.
> the URI is old page that is in bfcache.

That's not good at all.  What about the code at http://lxr.mozilla.org/seamonkey/source/layout/base/nsDocumentViewer.cpp#1390 (that comment, and the code below)?  That should be setting both the document and prescontext container to null.  Is that code not running?

Or is the problem that we're in a subframe and we never unset the container on subframes when we stick the toplevel page in bfcache?
(In reply to comment #25)
> (In reply to comment #24)
> > OK.  What's the documentURI of shell->GetDocument() ?  Is it the URI of the
> > page that's in bfcache, or of the newly-loaded page?
> > 
> 
> the URI is old page that is in bfcache.
> 

NOTE: the URI is latest document that is searched by FastFind.
I.e., the child document is pointed if the page is frame page.
That is, it seems to me that bug 292960 and bug 292961 are not resolved for subframes of the page actually being stored in bfcache...  That's not good.
(In reply to comment #26)
> That should be setting both the document
> and prescontext container to null.  Is that code not running?

No, the code is running.

> Or is the problem that we're in a subframe and we never unset the container on
> subframes when we stick the toplevel page in bfcache?

How much times running the code if the page has two frames?
I confirmed only one time at unloading the frame page.
Right.  So we never unset the containers, link handlers, etc on subframes.  That's not good at all.  We need to fix that for 1.8.
I don't have any opinion for fixing the bug of bfcache's bug.
But I think we need this patch too. Because current tricky mechanism is very *delicate* for other code's bugs. It's better way for always initializing the nsTypeAheadFind at the current document of the browser is unloaded.
And I have a additional info.
I think that Bug 307178 is similar bug. Please check it.
Comment on attachment 201254 [details] [diff] [review]
Patch rv2.0

ok, but I want bz to look at this too, if we're going to take this for 1.8
Attachment #201254 - Flags: superreview?(bzbarsky)
Attachment #201254 - Flags: review?(mconnor)
Attachment #201254 - Flags: review+
Comment on attachment 201254 [details] [diff] [review]
Patch rv2.0

I guess this is ok as a workaround... should I file a separate bug for the real bfcache issue here, then?  That needs to be fixed for 1.8 too.
Attachment #201254 - Flags: superreview?(bzbarsky) → superreview+
Filed bug 314549 on the underlying problem.
we should get the workaround release noted (refresh the page). not going to block on this since the steps aren't likely to be hit by a large number of users.
Flags: blocking1.8rc1? → blocking1.8rc1-
checked-in to Trunk.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment on attachment 201254 [details] [diff] [review]
Patch rv2.0

The risk is very low.
Attachment #201254 - Flags: approval1.8rc1? → approval1.8rc2?
Comment on attachment 201254 [details] [diff] [review]
Patch rv2.0

Given the easy workaround and the possibility of a global fix for underlying problem - we are not going to take this patch for 1.8
Attachment #201254 - Flags: approval1.8rc2? → approval1.8rc2-
Not sure when this was fixed- appear still to be an issue in RC1 for me (originally opened bug 313005). Thanks!
This bug is not fixed on 1.8 branch. This is fixed on trunk builds only.
Not sure what 1.8 vs. trunk means. Since the status is set to resolved (sorry for asking if the question is stupid)- will this be fixed for FF 1.5? Anything I could do?
> Not sure what 1.8 vs. trunk means.

1.8 is the Gecko in Firefox 1.5.  Trunk is work on Gecko 1.9, which will be in some future Firefox release.

> Since the status is set to resolved

That means it's fixed on trunk.

> will this be fixed for FF 1.5?

That requires driver approval, which was denied since the 1.8 code is basically in code freeze and has been for a few days now.  So "probably not, unless bug 314549 gets fixed".
Comment on attachment 201254 [details] [diff] [review]
Patch rv2.0

Bug 314549 cannot block 1.5 relase. We should use this workaround.
I'm re-requesting approval.
Attachment #201254 - Flags: approval1.8rc2- → approval1.8rc2?
rerequesting blocking, given that part of the reason for rejecting it (a better fix in the other bug) has gone away. I'm not sure why the steps wouldn't be hit by a large number of users - because there aren't a large number of frames pages out there? because most people don't use fastfind a lot?
Flags: blocking1.8rc2?
voting this to be fixed in 1.5.
I see a lot of people complain this bug in BBS.

voting this to be blocking- fast find is key differentiator
Attachment #201254 - Flags: approval1.8rc2? → approval1.8rc2-
Asa: How about recording the rationale for minusing the patch?

/be
too late in the game for a non-blocker. We're essentially done except for one security bug and this issue isn't highly visible because it requires a user first find in a frame and then in the same tab visit another page and try again to find. The failure is not catastrophic (no dataloss, chrash, hang, security implications) and it also has a simple, though not terribly obvious workaround, which is to reload the page.
Flags: blocking1.8rc2? → blocking1.8rc2-
The checked-in patch 201254 raised the regression in bug 314819.
Blocks: 314819
No longer blocks: 314819
Flags: blocking1.8.1?
Flags: blocking-firefox2?
Masayuki, can you get this in soon on branch, with the appropriate regression fixes?
Flags: blocking-firefox2? → blocking-firefox2+
Hey, I have already created the patch for regression that is waiting your review. See bug 315653. That fixes bug 314819 too.
Masayuki, now that those regressions are fixed, can we get the fixes rolled up and landed on the branch?
Whiteboard: [needs branch rollup]
pushing out non-critical-path bugs to b2
Target Milestone: Firefox1.5rc1 → Firefox 2 beta2
Whiteboard: [needs branch rollup] → [needs branch rollup] [at risk]
Attached patch branch rollup — — Splinter Review 
Includes regression fixes from bug 314819 and bug 315653. This combination of changes has been on the trunk for nearly four months.
Attachment #229667 - Flags: approval1.8.1?
OS: Windows XP → All
Hardware: PC → All
Whiteboard: [needs branch rollup] [at risk] → [at risk]
Comment on attachment 229667 [details] [diff] [review]
branch rollup

a=drivers, please land on the 1.8.1 branch
Attachment #229667 - Flags: approval1.8.1? → approval1.8.1+
Whiteboard: [at risk] → [checkin needed (1.8 branch)][at risk]
mozilla/toolkit/content/widgets/browser.xml 	1.70.2.14
Keywords: fixed1.8.1
Whiteboard: [checkin needed (1.8 branch)][at risk]
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: