406697 - Can't load the correct bookmark in sidebar after opened another page in it.

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Po-chiang Chao [:bobchao]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007120204 Minefield/3.0b2pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007120204 Minefield/3.0b2pre

This bug is happened both on my XP and Linux (minefield build num is the same.)

If you loaded one bookmark in  sidebar before in the browser window, you can't load another page in it correctly -- the page loaded in sidebar will still be the previous one.

Reproducible: Always

Steps to Reproduce:
1. Add one bookmark (say, http://google.com) by dragging favicon to Bookmarks Toolbar, check "Load this bookmark in the sidebar" in the bookmark's Properties dialog.
2. Add another one bookmark (say, http://yahoo.com) by dragging favicon to Bookmarks Toolbar,  also check "Load this bookmark in the sidebar" in the bookmark's Properties dialog.
3. Click the 1st bookmark, the sidebar should open with Google loaded.
4. Close the sidebar.
5. Click the 2nd bookmark.
Actual Results:  
Google shows again.

Expected Results:  
Yahoo! should be loaded this time.

If you open another browser window and click the 2nd bookmark, it would be Yahoo! loaded correctly in sidebar, and then you can't load Google in the window's sidebar. This problem didn't happened in Firefox 2.

Comment 1

[Al Billings [:abillings - ex-MoCo]]

I'm seeing this is OS X as well with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b2pre) Gecko/2007120304 Minefield/3.0b2pre.

Comment 2

[Tracy Walker [:tracy]]

confirmed - notice after step 5 that the title of the sidebar is Yahoo! but the content remains Google. New Window behavior is funky as well.  

Requesting blocking Fx3 - we should be able to consecutively load sidebar loading bookmarks.

Comment 3

[Ria Klaassen (not reading all bugmail)]

Regression range for this is http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1196066040&maxdate=1196067059 so Bug 385224 could have caused this.

Comment 4

[Felix Hahn]

I'm having this problem too. It first appeared in build 2007112605.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Yup, this is a regression from bug 385224.  Looking...

Comment 6

[Myk Melez [:myk] [@mykmelez]]

This is a regression from the change to browser.xml in bug 385224.  That change makes browsers cache their docShell objects to speed up things that access them.

That's OK for the browsers in the main tabbrowser, which retain the same docShell for their entire lifetime, as far as I can tell.  But it's not OK for the sidebar browser, which gets a new docShell every time you hide and show it.

And so the cache is stale the second time you open the sidebar, and the code in openWebPanel that is responsible for loading the new URL fails, although it does succeed in first updating the sidebar title, which is why the sidebar title is correct at the end of the testcase.

I know of two fixes to this bug:

1. Revert the change that added the docShell cache.  This will impact performance, although it's not clear how much.  The docShell cache was just one of a number of optimizations in the fix for bug 385224.

2. Extend the browser widget with a version that uses the cache and only bind the tabbrowser browsers to that version.

cc:ing some layout folks, though, in case there's a third solution (f.e. perhaps browser widgets shouldn't be getting new docShells every time they get hidden and reshown, and we can fix that bug instead).

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Attachment: fix 1: revert the optimization from bug 385224

Here's the first of the two possible fixes I identified in comment 6.  This fix just reverts the optimization from bug 385224, sacrificing that performance win to fix this bug.  This is the lower-risk fix.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: fix 2: special-case tabbrowser browsers

Here's the second of the two possible fixes I identified in comment 6.  This fix retains the performance optimization for the main tabbrowser widget while fixing this bug in the sidebar browser widget by making the cache apply specifically to browsers that are children of the #content node.  This is the higher-risk fix.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Update: Neil Rashbrook on IRC says that XUL browser/iframe elements intentionally destroy their docShells when they get hidden, so that's not a bug.

A third fix might be to invalidate the cache when the element gets hidden, but it's not clear how to detect that the element has been hidden.  We could check on every unload/pagehide, but the cost of that might just obviate the perf gain from caching the docShell in the first place.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Neil Rashbrook on IRC says that XUL browser/iframe elements
> intentionally destroy their docShells when they get hidden, so that's not a
> bug.

Actually, it is.  I was sure we have it filed, but I can't find it...  The docshell should really be owned by the binding, not the frame or something.  The current setup has a number of problems that would be fixed by that change.

That's not a 1.9 kind of change at this point, probably.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Attachment: fix 3: invalidate the docshell cache on browser hide

Per comment 9, here's the third possible fix.  This retains the docshell cache but invalidates it on pagehide if the pagehide is occurring because the browser has been hidden.  This should retain some of the performance win of the original fix that caused the regression, although it's not clear how much.  As a side-effect, it includes the typo fix followup patch in bug 407116.  This is the middling-risk fix.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Attachment: fix 3 unrotted

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #12)
> Created an attachment (id=296089) [details]
> fix 3 unrotted
> 

This fix looks pretty safe. This bug has been causing more than a few problems in extensions as well. We should get this problem resolved sooner than later with more extensions updating to Fx3.

At the very least we should get fix 1 landed (reverts the optimization from bug 385224) and see how much the pref regresses.

Comment 18

[Tracy Walker [:tracy]]

If it hasn't rotted, can we get that fix reviewed and checked-in for b4

Comment 19

[max1million]

same on 3.0b3

on webpanel open:
Error: this.docShell is null
Source File: chrome://global/content/bindings/browser.xml
Line: 0

on webpanel closed:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebProgress.removeProgressListener]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://browser/content/web-panels.js :: unload :: line 87"  data: no]

on webpanel re-open:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebNavigation.document]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://global/content/bindings/browser.xml :: get_contentDocument :: line 0"  data: no]

result: opens last page from the first time it was open

version:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3) Gecko/2008020513 Firefox/3.0b3

Comment 20

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

Talked to bz and smaug about this on IRC today, they pointed out that bug 395609 will fix this by fixing the bug bz describes in comment 10. I've confirmed that the latest patch in that bug fixes this one, and that bug is a blocker, so we can just wait until that lands and avoid having to take any of the patches in this bug.

Comment 22

[Mike Connor [:mconnor]]

Confirmed fixed on trunk.

Comment 23

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b5pre) Gecko/2008032005 Minefield/3.0b5pre] (nightly) (W2Ksp4)

V.Fixed (behavior from comment 0, 1 strict warning and 2 exceptions from comment 19) by
{{
2008-03-21 04:18	Olli.Pettay%helsinki.fi 	...  	Bug 395609, r=roc+sicking, sr=sicking, a=beltzner
}}

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b5pre) Gecko/2008032106 Minefield/3.0b5pre] (nightly) (W2Ksp4)