Closed Bug 245327 Opened 21 years ago Closed 21 years ago

theme switch clears all tabs, disables their history, can't close these tabs

Categories

(Toolkit :: Add-ons Manager, defect, P2)

Product:
Toolkit
Component:
Add-ons Manager
Platform:
x86
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: xanthor+bz, Assigned: dbaron)

References

Details

(Keywords: dataloss, fixed-aviary1.0, fixed1.7.5)

Attachments

(1 file, 1 obsolete file)

do root element fixups earlier (before CalcDifference)
4.66 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a2) Gecko/20040601 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a2) Gecko/20040601 Firefox/0.8.0+

When I try to install a theme or to change one (with the theme manager), all
tabs become "about:blank" and lose their history : I can't recover the pages I
was reading...

Reproducible: Always
Steps to Reproduce:
1. Install a theme (for example with http://texturizer.net/firefox/themes/ )
or
1bis. Change the theme used
Actual Results:  
All the tabs opened are "cleaned" : they display `about:blank` and have no history

Expected Results:  
No change on tabs opened...

(It occures to me also once with Mozilla 1.7a I think. So maybe it's a dupe,
even if I haven't find anything related...)
I ran into the exact same issue when installing the Mostly Crystal theme.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a2) Gecko/20040602 Firefox/0.8.0+
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a2) Gecko/20040605 Firefox/0.8.0+
(daihard: XFT+GTK2; opt. for P4/SSE-2)

Confirmed. This has been happening for a LONG time, but only with the old-style TM.
I can reproduce this with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.7) Gecko/20040608 Firefox/0.8.0+ (Steffen). (Please test the
extension/theme manager with branch builds, the trunk isn't up-to-date).

Switching to any theme, including Ben's "New Theme 1", blanks all open tabs and
makes them unresponsive. You can't load any pages, and you can't close the tabs.

Workaround is to open a new window and close the old one.
Severity: normal → major
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking0.9?
OS: Linux → All
(In reply to comment #4)
> (Please test the extension/theme manager with branch builds, the trunk isn't 
> up-to-date).

As you noted in bug 245885, the EM was broken in my build. I was simply letting
the bug know that this issue has been present for many months, well back into
the early weeks of the 0.8.0+ trunk builds and the old-style theme system.
I was wrong in saying that the tabs are useless. You can load webpages in them,
but their history is broken/disabled and you can't close them. Adjusting summary
and requesting blocking0.9.

Further testing showed that merely installing a theme is harmless, probably
since Ben disabled the "and swith to it" checkbox.

NB: I noticed that everyone in here reported using trunk builds. I just wanted
to give a hint that it's much more useful to use branch builds. No offense intended.
Summary: Installing or changing theme clear all tabs and reset their history → theme switch clears all tabs, disables their history, can't close these tabs
I can reproduce as well on Windows XP with Fire Fox 0.9RC

If only 2 tabs are open at the time closing 1 tab will close all tabs (2 of them
at max). Then exiting the browser will still prompt for the tabs to be closed,
even if they arent open.

If more tabs than two are open (3+) at the time  the theme is changed, The error
Steffen Wilberg descibes follows.

In both instances the tabs are set to blank pages.
Flags: blocking0.9? → blocking0.9-
Flags: blocking1.0?
Confirmed with my branch build on Linux:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040609 Firefox/0.8.0+
(daihard: XFT+GTK2; opt. for P4/SSE-2)
I swear I saw this before.
Whiteboard: DUPEME
This is a very serious dataloss issue and I don't think it should be duped to
bug 243717 because then we'll loose the blocking1.0 flag. Personally, I can't
understand how this bug can be accepted in the 0.9 release.
Whiteboard: DUPEME
Keywords: dataloss
*** Bug 246799 has been marked as a duplicate of this bug. ***
(In reply to comment #11)
> This is a very serious dataloss issue and I don't think it should be duped to
> bug 243717 because then we'll loose the blocking1.0 flag. Personally, I can't
> understand how this bug can be accepted in the 0.9 release.

I agree. My understanding is that the only workaround is to make sure no tabs
containing important information are open when you switch themes. Even that does
not prevent the history from being lost.
This bug just hit me while installing the Qute theme for Mozilla/5.0 (Windows;
U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040614 Firefox/0.9, and it is probably
the most serious bug I have ever seen creep into a major
Phoenix/Firebird/Firefox release. I would heavily recommend altering the bug
priority to 'blocking' as soon as possible, as it is extremely likely that new
Firefox users will become aware of this as they begin to explore
http://update.mozilla.org/.
This bug hit me for the first couple of theme installs.  I think this was with a
prior build than what I am using now.  I installed the Qute theme today and I
didn't get it for some reason.  The theme switch still wasn't completely
successful though because not all buttons changed (ie. I had a mix between Smoke
theme and Qute theme buttons).

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040613
Firefox/0.8.0+
Happened here too, both with Qute and Apollo. Note though that while the tabs
lose MOST things, they DO NOT lose the website's icon. For example, if I were to
install a theme w/ bugzilla open, I would have an empty tab w/ the bugzilla
raptor icon still on the left of the tab.

XP SP1
*** Bug 247279 has been marked as a duplicate of this bug. ***
Just as an FYI, the _Linux Today_ editor gave Firefox a try and wrote a quick
review in which he mentioned experiencing this bug.  Thanks go to daihard from
the Mozillazine forums for the shout:

http://linuxtoday.com/infrastructure/2004061802026RVSWNT
(In reply to comment #18)
> Just as an FYI, the _Linux Today_ editor gave Firefox a try and wrote a quick
> review in which he mentioned experiencing this bug.  Thanks go to daihard from
> the Mozillazine forums for the shout:
> 
> http://linuxtoday.com/infrastructure/2004061802026RVSWNT

It's me! ;)

Seriously... I agree with Jeff that this one should be considered one of the
showstoppers for the 1.0 release. It does not go as far as crashing Firefox, but
losing all the tabbed contents with no way of recovering them is bad enough. Now
that Linux Today mentioned it, even more people know about it (and expect it to
be fixed).
(In reply to comment #15)
> The theme switch still wasn't completely
> successful though because not all buttons changed (ie. I had a mix between
Smoke > theme and Qute theme buttons).
> 
I can confirm this problem: the behaviour seems to be different with different
themes. For example when I installed Charamel the toolbars changed color but
icons were still that of the previous theme; with Mostly Crystal the theme
seemed to change everything.

Afeter theme switching you can't close existing tabs; if you open new tabs, they
work normally.

Another thing that nobody has pointed out: when you switch theme the
Previos/Next button stay inactive and don't work at all.
Severity: major → critical
*** Bug 247939 has been marked as a duplicate of this bug. ***
I have very little experience with the Mozilla code, so I'm not sure where
exactly the problem is, but here's a rough picture of what's going on I've
gotten from adding debug messages all over the place (in Linux, using Aviary CVS) -

1.  In the process of changing the theme, a bunch of chrome style sheets get
reloaded.  After doing this, the chrome registry calls the DOM document's
UpdateStyleSheets method.
2.  The document updates its style sheets and calls EndUpdate on all of its
observers afterward.  One of the observers is a PresShell that calls its
CSSFrameConstructor's ProcessRestyledFrames method.  Seems logical to me so far.
3.  The style changes turn out to be such that ReconstructDocElementHierarchy
ends up getting called.
4.  The call to RemoveFrame in ReconstructDocElementHierarchy starts a chain of
calls to the Destroy methods of various objects and the session history happens
to be one of them.  I'm not sure where the URL of the current frame gets lost,
but it's probably somewhere in here.
4(b).  If you really want to have fun, you can change |if (container) {| to |if
(0) {| in nsCSSFrameConstructor::ProcessRestyledFrames (line 11313 of
nsCSSFrameConstructor.cpp) and watch your session history get fried whenever you
load a page with a new style sheet.  Try mozilla.org/firefox for an example -
the page will go blank just after it finishes loading.  This also seems to turn
up some nasty exception that tells me to run gdb, however, so it's only
recommended if you like seeing things break.

I've also noticed that the proper style sheets never get reloaded the first time
I run a build - the toolbar buttons stay the same and the session history is
untouched - which is probably a separate bug.  Could be a build configuration
issue of mine, though.


Hopefully, this will turn out to be helpful.  I'll try testing on the trunk next
and see if I get the same behavior.
Ugh.  In 4(b), the if is in nsCSSFrameConstructor::RecreateFramesForContent, not
ProcessRestyledFrames.  D'oh.
*** Bug 248331 has been marked as a duplicate of this bug. ***
Blocks: 226791
*** Bug 241614 has been marked as a duplicate of this bug. ***

*** This bug has been marked as a duplicate of 226791 ***
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → DUPLICATE
*** Bug 248778 has been marked as a duplicate of this bug. ***
Removing blocking request since this is marked as a dupe of bug 245327.
Flags: blocking-aviary1.0?
(In reply to comment #28)
> Removing blocking request since this is marked as a dupe of bug 245327.

surely, you wanted to say "... marked as a dupe of bug 226791"
Er, yes, indeed!  :)
Blocks: 134260
No longer blocks: 226791
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Status: REOPENED → NEW
Depends on: 249655
I confirm this with Windows XP Professional SP1a and Mozilla/5.0 (Windows; U;
Windows NT 5.1; en-US; rv:1.7) Gecko/20040614 Firefox/0.9.

I may use blank tabs to load new pages (e.g. pressing buttons on Bookmarks
Toolbar), though Reload does not work.

If there are more Firefox windows with tabs, they are affected too.

How to reproduce
1. Open 2 tabs and use them to view pages
2. Tool -> Themes
3. Select a different theme and press "Use Theme".

Themes tested:
* Firefox (default) 2.0
* Qute 2.1.1
* Curacao 1.0

Also: reentering the URL in the blanked tab reloads the page, but the scrollbars
still look blank (right now, with Curacao 1.0).
*** Bug 249668 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.0?
I have experienced the same problem. 
Greg, what's calling ReconstructDocElementHierarchy?  Is it called from
nsCSSFrameConstructor::RecreateFramesForContent?

If so, could you use DOM inspector to look at the rules applied to the root node
in the XUL document both before and after the theme switch and see what the
differences are?
*** Bug 251188 has been marked as a duplicate of this bug. ***
(In reply to comment #34)
> Greg, what's calling ReconstructDocElementHierarchy?  Is it called from
> nsCSSFrameConstructor::RecreateFramesForContent?

Yes, it's nsCSSFrameConstructor::RecreateFramesForContent.  My first post says
ProcessRestyledFrames because that's what calls RecreateFramesForContent and I
momentarily confused the two.

> If so, could you use DOM inspector to look at the rules applied to the root node
> in the XUL document both before and after the theme switch and see what the
> differences are?

Breaking this down into something an extreme n00b like myself can easily follow,
do you mean to point the DOM inspector at chrome://browser/content/browser.xul
and look at the CSS Style Rules for the window node just below #document?  If
so, I've seen differences there when the session history is unaffected (and the
toolbar buttons are as well), but I've also seen no differences there when the
session history gets dumped (and the toolbar buttons get updated properly).  In
fact, I modified extensions.js so I could reload the current theme and found
that to be capable of destroying the session history as well.  If that's not
what you meant, I'm sorry for needing to have everything spelled out to me, but
I'm afraid I do.
> do you mean to point the DOM inspector at chrome://browser/content/browser.xul
> and look at the CSS Style Rules for the window node just below #document?

Yes.  That's exactly what I meant.

> In fact, I modified extensions.js so I could reload the current theme and found
> that to be capable of destroying the session history as well.

Hmm.  How many times is nsPresShell::EndUpdate actually being called when you do
that?  What's the nodename of the node that ends up triggering the
ReconstructDocElementHierarchy call when we try to reframe it?
(In reply to comment #37)
> Hmm.  How many times is nsPresShell::EndUpdate actually being called when you do
> that? 

It might be a few less that when I actually switch themes, but it's hard to tell
because nsPresShell::EndUpdate also gets called when I move the mouse over
something and that makes it tough to count.

> What's the nodename of the node that ends up triggering the
> ReconstructDocElementHierarchy call when we try to reframe it?

window.
Greg, how many times is it called with UPDATE_STYLE as the argument?  It should
only be getting called once that way; if it's getting called more times than
that, that could easily lead to this bug...  If it's being called more than
once, I'd be very interested in the callers of nsDocument::EndUpdate that pass
UPDATE_STYLE.
nsChromeRegistry::RefreshWindow is called three times when, "Use Theme," is
clicked, one of which is recursive.  Each of these three times,
nsPresShell::EndUpdate(UPDATE_STYLE) gets called exactly once.  This behavior is
the same whether the session history gets destroyed or not.
Hmm..  So each actual update only calls EndUpdate() once, after actually
updating the sheets.  Given that, we really shouldn't be ending up in a
situation where the hint is anything but "none" for the case when we switch to
the same theme...

Is there a way to enable this theme-switching stuff in SeaMonkey so I can try to
test it?
Well, I'm not sure what to do about the whole window, but I did succeed in
getting ReconstructDocElementHierarchy called on the Preferences window by going
into pref-themes.js and doing something like this:

    function applySkin()
    {
      ...

      var inUse = reg.isSkinSelected(data.name, true);
      if (!theme && inUse == Components.interfaces.nsIChromeRegistry.FULL) return;

      parent.hPrefWindow.setPref("string", "general.skins.selectedSkin", data.name);

++    //Trigger a style sheet reload.
++    reg.refreshSkins();
  
      ...
    }

The Preferences window will cease to allow you to do much of anything and the
reframe appears to be triggered by a node called, "dialog."  pref-themes.js is
in xpfe/components/prefwindow/resources/content if you want to modify it in your
source tree or comm.jar!/content/communicator/pref if you don't feel like doing
another make install.
Maybe you also want
        reg.selectSkin(data.name, true);
http://lxr.mozilla.org/aviarybranch/source/browser/components/prefwindow/content/pref-themes.js#138
(that's Firefox's old theme manager)

> ++    //Trigger a style sheet reload.
> ++    reg.refreshSkins();
(In reply to comment #43)
> Maybe you also want
>         reg.selectSkin(data.name, true);

Probably.  I think I tried that and it did something I didn't like, but now I
can't remember what that something was :/

Other stuff:
Is the display style for the root window node supposed to be block or -moz-box?
 If I look at the Computed Style in the DOM Inspector, I see block in Firefox
0.9 and recent CVS from Aviary and SeaMonkey, but -moz-box in Firefox 0.8 and
Mozilla 1.6.  The reason I ask is that, after throwing in a bunch of debugging
messages, I found that in the CaptureChange function in nsFrameManager.cpp,
something is showing a change of display type from 1 to 18 and the name of the
content node appears to be, "window."  The computed style for the root node is
still block after a theme change, though.
Aha!  The issue is the code in nsCSSFrameConstructor::ConstructDocElementFrame
that implements section 9.2.4 of CSS2.1.  Since this fixup doesn't happen at
style resolution time, you get a reframe hint (1 is "block" and 18 is "-moz-box"
for display values).

As a quick-fix we could make this code not muck with a root having -moz-box
display at
http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSFrameConstructor.cpp#3506

That said, couldn't we change things so that this fixup would happen in
nsRuleNode any time the style context has no parent context?  That should only
happen for the root, I would think.
Should it be there or in nsStyleContext::ApplyStyleFixups?

Also, shouldn't we allow the root to be -moz-box and construct a
DocElementBoxFrame accordingly?
Putting it in nsStyleContext::ApplyStyleFixups sounds ok to me (I'm not sure why
we fixup both there and in the rulenode, though).

And yes, I agree that we should allow the root to be a -moz-box (and fix up the
other XUL types to -moz-box, perhaps?)
I actually don't want to mess with the box-as-root for now, since we don't have
a proper initial containing block setup, and it would introduce new crashes that
we don't have now.

And I realized that it would really break things if we did it in nsRuleNode,
since if the root were later reresolved we could end up with a context without
the fixup, unless we forced everything that were not 'none', 'block', or 'table'
to be stored in the style context tree instead of the rule tree (i.e.,
'inherited = PR_TRUE').

I'll attach a patch shortly.
This version compiles, and fixes the bug.  It exposes some problems with
dynamic changes to -moz-image-region, though.
Assignee: bugs → dbaron
Attachment #153432 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #153452 - Flags: superreview?(bzbarsky)
Attachment #153452 - Flags: review?(bzbarsky)
Comment on attachment 153452 [details] [diff] [review]
do root element fixups earlier (before CalcDifference)

Looks good.  r+sr=bzbarsky.
Attachment #153452 - Flags: superreview?(bzbarsky)
Attachment #153452 - Flags: superreview+
Attachment #153452 - Flags: review?(bzbarsky)
Attachment #153452 - Flags: review+
Flags: blocking-aviary1.0RC1+
Flags: blocking-aviary1.0?
Flags: blocking-aviary1.0+
Priority: -- → P2
Fix checked in to trunk, 2004-07-17 11:21 -0700.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago → 21 years ago
Resolution: --- → FIXED
This should be reopened unitil fixed on the branch too.
Nope.  The Status and Resolution fields are used to track status on the trunk;
keywords are used for branches.
Since I see no keywords, has this also been fixed on branch?
Whiteboard: needed-aviary1.0
Nope.  Would you also assume from the lack of keywords that it's been fixed on
the 1.4 branch?  The 1.0 branch?  The M18 branch?

(That said, I'm planning to land it on the Aviary branch once the Aviary branch
reopens.)
Fix checked in to AVIARY_1_0_20040515_BRANCH, 2004-07-22 12:45 -0700.
Keywords: fixed-aviary1.0
Whiteboard: needed-aviary1.0
*** Bug 252952 has been marked as a duplicate of this bug. ***
*** Bug 253335 has been marked as a duplicate of this bug. ***
*** Bug 254160 has been marked as a duplicate of this bug. ***
what about 1.7?
Keywords: fixed1.7.5
Product: Firefox → Toolkit
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: