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)
Tracking
()
RESOLVED
FIXED
mozilla1.8.1beta2
People
(Reporter: masayuki, Assigned: masayuki)
References
Details
(Keywords: fixed1.8.1, regression)
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
mconnor
:
review+
bzbarsky
:
superreview+
asa
:
approval1.8rc2-
|
Details | Diff | Splinter Review |
(deleted),
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.
Assignee | ||
Comment 1•19 years ago
|
||
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051028 Firefox/1.6a1
Assignee | ||
Comment 2•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Severity: normal → major
Assignee | ||
Comment 3•19 years ago
|
||
I can reproduce with 1.8 branch!
This is serious!
Version: Trunk → 1.5 Branch
Assignee | ||
Updated•19 years ago
|
Flags: blocking1.8rc1?
Assignee | ||
Comment 4•19 years ago
|
||
regressed between 2005062806 and 2005062906
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-06-28+06%3A00%3A00&maxdate=2005-06-29+06%3A00%3A00&cvsroot=%2Fcvsroot
Comment 5•19 years ago
|
||
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.
Assignee | ||
Comment 6•19 years ago
|
||
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
Assignee | ||
Updated•19 years ago
|
Summary: FastFind is broken after find in this page in frame page(?) → FastFind is broken after find in this page in frame page
Assignee | ||
Comment 7•19 years ago
|
||
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?
Assignee | ||
Updated•19 years ago
|
Priority: -- → P1
Target Milestone: --- → Firefox1.5rc1
Assignee | ||
Comment 8•19 years ago
|
||
I'll be away 5 or 6 hours. Sorry.
Comment 9•19 years ago
|
||
Comment on attachment 201233 [details] [diff] [review]
Patch rv1.0
hrm, shouldn't it be done on-page-show?
Assignee | ||
Updated•19 years ago
|
Attachment #201233 -
Flags: review?(mconnor)
Attachment #201233 -
Flags: review-
Attachment #201233 -
Flags: approval1.8rc1?
Assignee | ||
Comment 10•19 years ago
|
||
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?
Comment 11•19 years ago
|
||
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?
Assignee | ||
Comment 12•19 years ago
|
||
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?
Assignee | ||
Comment 13•19 years ago
|
||
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.
Comment 14•19 years ago
|
||
> 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?
Comment 15•19 years ago
|
||
(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.
Assignee | ||
Comment 16•19 years ago
|
||
(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.
Comment 17•19 years ago
|
||
> 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.
Assignee | ||
Comment 18•19 years ago
|
||
mPresShell and mDocShell are always set with do_GetWeakReference in nsTypeAheadFind.cpp...
Comment 19•19 years ago
|
||
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?
Assignee | ||
Comment 20•19 years ago
|
||
(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() ?
Comment 21•19 years ago
|
||
Er, yes. I wish the accessor and the member names matched....
Assignee | ||
Comment 22•19 years ago
|
||
shell->GetDocument()->GetContainer() is not null.
Comment 23•19 years ago
|
||
(In reply to comment #11)
>Does XPFE need similar changes?
XPFE doesn't try to do anything tricky with fast find.
Comment 24•19 years ago
|
||
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?
Assignee | ||
Comment 25•19 years ago
|
||
(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.
Comment 26•19 years ago
|
||
> 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?
Assignee | ||
Comment 27•19 years ago
|
||
(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.
Comment 28•19 years ago
|
||
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.
Assignee | ||
Comment 29•19 years ago
|
||
(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.
Comment 30•19 years ago
|
||
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.
Assignee | ||
Comment 31•19 years ago
|
||
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.
Assignee | ||
Comment 32•19 years ago
|
||
And I have a additional info.
I think that Bug 307178 is similar bug. Please check it.
Updated•19 years ago
|
Blocks: blazinglyfastback
Comment 33•19 years ago
|
||
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 34•19 years ago
|
||
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+
Comment 35•19 years ago
|
||
Filed bug 314549 on the underlying problem.
Comment 36•19 years ago
|
||
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-
Assignee | ||
Comment 37•19 years ago
|
||
checked-in to Trunk.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 38•19 years ago
|
||
Comment on attachment 201254 [details] [diff] [review]
Patch rv2.0
The risk is very low.
Attachment #201254 -
Flags: approval1.8rc1? → approval1.8rc2?
Comment 39•19 years ago
|
||
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-
Comment 40•19 years ago
|
||
Not sure when this was fixed- appear still to be an issue in RC1 for me (originally opened bug 313005). Thanks!
Assignee | ||
Comment 41•19 years ago
|
||
This bug is not fixed on 1.8 branch. This is fixed on trunk builds only.
Comment 42•19 years ago
|
||
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?
Comment 43•19 years ago
|
||
> 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".
Assignee | ||
Comment 44•19 years ago
|
||
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?
Comment 45•19 years ago
|
||
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?
Comment 46•19 years ago
|
||
voting this to be fixed in 1.5.
I see a lot of people complain this bug in BBS.
Comment 47•19 years ago
|
||
voting this to be blocking- fast find is key differentiator
Updated•19 years ago
|
Attachment #201254 -
Flags: approval1.8rc2? → approval1.8rc2-
Comment 48•19 years ago
|
||
Asa: How about recording the rationale for minusing the patch?
/be
Comment 49•19 years ago
|
||
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.
Updated•19 years ago
|
Flags: blocking1.8rc2? → blocking1.8rc2-
Comment 50•19 years ago
|
||
The checked-in patch 201254 raised the regression in bug 314819.
Blocks: 314819
Updated•19 years ago
|
Flags: blocking1.8.1?
Updated•19 years ago
|
Flags: blocking-firefox2?
Comment 51•19 years ago
|
||
Masayuki, can you get this in soon on branch, with the appropriate regression fixes?
Flags: blocking-firefox2? → blocking-firefox2+
Assignee | ||
Comment 52•19 years ago
|
||
Hey, I have already created the patch for regression that is waiting your review. See bug 315653. That fixes bug 314819 too.
Comment 53•19 years ago
|
||
Masayuki, now that those regressions are fixed, can we get the fixes rolled up and landed on the branch?
Updated•19 years ago
|
Whiteboard: [needs branch rollup]
Comment 54•19 years ago
|
||
pushing out non-critical-path bugs to b2
Target Milestone: Firefox1.5rc1 → Firefox 2 beta2
Updated•19 years ago
|
Whiteboard: [needs branch rollup] → [needs branch rollup] [at risk]
Comment 55•19 years ago
|
||
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?
Updated•19 years ago
|
OS: Windows XP → All
Hardware: PC → All
Whiteboard: [needs branch rollup] [at risk] → [at risk]
Comment 56•19 years ago
|
||
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+
Updated•19 years ago
|
Whiteboard: [at risk] → [checkin needed (1.8 branch)][at risk]
Comment 57•19 years ago
|
||
mozilla/toolkit/content/widgets/browser.xml 1.70.2.14
Keywords: fixed1.8.1
Whiteboard: [checkin needed (1.8 branch)][at risk]
Updated•16 years ago
|
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•