persist="selectedIndex" no longer working for tabbox elements

RESOLVED FIXED in mozilla1.9.1b1

Status

()

Core
XBL
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: Kaspar Brand, Assigned: mayhemer)

Tracking

(Depends on: 1 bug, {regression})

Trunk
mozilla1.9.1b1
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
With this change to nsPresShell.cpp, a regression for tabbox was introduced:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsPresShell.cpp&branch=&root=/cvsroot&subdir=mozilla/layout/base&command=DIFF_FRAMESET&rev1=3.1056&rev2=3.1057

> Process XBL constructors after processing style reresolves. In particular,
> this makes sure that we process the former even if there were none of the
> latter. Bug 394676 and bug 394014

Since 2007-09-14, persist="selectedIndex" is broken for tabbox elements (as demonstrated by cert manager - see bug 373525): when the tabbox is opened, it will always show the content of the first tab. Switching between two tabs will correctly refresh the content, however.

(To reproduce the problem, use a recent trunk build, open cert manager from the security preferences ["View Certificates"], select one of the tabs 2..5, close cert manager and reopen - it will always open with the contents of "Your Certificates".)
Can you create a testcase that's smaller than the entire cert manager, by any chance?  That would really help with getting this fixed...
Blocks: 394014, 394676
Flags: blocking1.9?
(Reporter)

Comment 2

10 years ago
Created attachment 283367 [details]
test case

(In reply to comment #1)
> Can you create a testcase that's smaller than the entire cert manager, by any
> chance?

Yes, here is one, which can be tested in any browser window (I've tried recent trunk builds from both Firefox and Seamonkey).

To easily reproduce the effect, select the second tab ("Two") and then reload the document - while the "Two" tab will stay selected, its content is then replaced by that of tab ONE. The same happens if you select tab two, change to another URL and then come back.

An interesting thing to note is that it's actually the presence of the treecol element on the first tab which triggers the bug - as soon as this one is removed, the problem will disappear...

Finally - fwiw, removing the

  mDocument->BindingManager()->ProcessAttachedQueue();

line from nsPresShell.cpp would "fix the problem", though that's probably not what you want to do, I assume.
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a9pre) Gecko/2007100303 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Checking testcase with this nightly (and new profile),
which is before "2007-10-03 10:21 / kaie%kuix.de / Bug 373525" and "2007-10-03 16:38 / bzbarsky%mit.edu / Bug 394676":
no bug, persistence works fine.

But I get errors like
[
Error: uncaught exception: Permission denied to get property XPCComponents.classes

Security Error: Content at chrome://global/content/bindings/tree.xml may not load data from https://bugzilla.mozilla.org/attachment.cgi?id=283367.

Error: this.view has no properties
Source File: chrome://global/content/bindings/tree.xml
Line: 0
]
which may be other bugs !?
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a9pre) Gecko/2007100304 Minefield/3.0a9pre] (nightly) (W2Ksp4)

Same behavior: testcase works and getting
[
Error: uncaught exception: Permission denied to get property
XPCComponents.classes
]
Status: NEW → ASSIGNED
Status: ASSIGNED → NEW
(Reporter)

Comment 5

10 years ago
(In reply to comment #3)
> Checking testcase with this nightly (and new profile),
> which is before "2007-10-03 10:21 / kaie%kuix.de / Bug 373525" and "2007-10-03
> 16:38 / bzbarsky%mit.edu / Bug 394676":
> no bug, persistence works fine.

Sigh. Yes, you're correct - it was fixed by this commit to nsPresShell.cpp (on 2007-10-01):

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsPresShell.cpp&branch=&root=/cvsroot&subdir=mozilla/layout/base&command=DIFF_FRAMESET&rev1=3.1063&rev2=3.1064

> But I get errors like
> [
> Error: uncaught exception: Permission denied to get property
> XPCComponents.classes
> 
> Security Error: Content at chrome://global/content/bindings/tree.xml may not
> load data from https://bugzilla.mozilla.org/attachment.cgi?id=283367.
> 
> Error: this.view has no properties
> Source File: chrome://global/content/bindings/tree.xml
> Line: 0
> ]
> which may be other bugs !?

Mostly cosmetic - they will disappear if you load the test case as a chrome URI (try adding the file to toolkit.jar, with content/global/test-398289.xul as its relative pathname, and then open chrome://global/content/test-398289.xul).

Boris, how "safe" is it to resolve this bug? Any other upcoming changes to nsPresShell in the next few days, which might reintroduce this regression...? ;-)
Well, I'd just gotten that same un-regression range and was wondering what patch caused it...  ;)

The reason that patch fixed the bug is that without it, creating the  nsTreeColFrame under that ContentInserted call that InitialReflow makes was causing a layout flush (from nsTreeBoxObject::GetTreeBody, called by nsTreeBoxObject::GetColumns, called by nsTreeColFrame::InvalidateColumns), which ran XBL constructors, so the constructor of the tabbox ran too early, I presume, before all the necessary things were in place.  So its selectedIndex munging failed.

The change to not allow flushing under there was purposeful, so I would certainly hope it won't regress!  But just to make sure, it would be great to turn this testcase into a regression test...  I wonder whether it would be doable using reftest with the XUL in a big enough iframe.

Kaspar, do you want to give that a shot, or should I?
Depends on: 398108
Flags: in-testsuite?
(Reporter)

Comment 7

10 years ago
(In reply to comment #6)
> But just to make sure, it would be great to
> turn this testcase into a regression test...  I wonder whether it would be
> doable using reftest with the XUL in a big enough iframe.
> 
> Kaspar, do you want to give that a shot, or should I?

I'll gladly leave that to you, since I'm not very experienced in all the XUL/XBL/reftest stuff, so you're welcome to take over ;-) Thanks!
So it sounds like this is fixed? If not, feel free to renominate.
Flags: blocking1.9?
Checked in a reftest for this.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
(Reporter)

Comment 10

10 years ago
(In reply to comment #9)
> Checked in a reftest for this.

Thanks! In the meantime I learned at least how to *run* reftests and was curious to see if it would really fail for trunk builds from 2007-09-15 to 2007-09-01... and in the current version it does not, unfortunately.

What needs to be added is setting the selectedIndex to 0 either after 398289-1.html is unloaded (window.onunload), or when 398289-1-ref.html is loaded.

"Like so", actually ;-)

Index: 398289-1-ref.html
===================================================================
RCS file: /cvsroot/mozilla/layout/reftests/bugs/398289-1-ref.html,v
retrieving revision 1.2
diff -u -r1.2 398289-1-ref.html
--- 398289-1-ref.html   6 Oct 2007 05:21:03 -0000       1.2
+++ 398289-1-ref.html   8 Oct 2007 17:39:04 -0000
@@ -3,6 +3,7 @@
 <head>
   <script>
     window.onload = function () {
+      window.frames[0].document.getElementById("test").selectedIndex = 0;
       window.frames[0].document.getElementById("test").selectedIndex = 1;
       document.documentElement.className = "";
     }

Boris, can you add this change (or should I add a patch as an attachment, because it needs reviews/approval)?
Hmm.  It was failing over here...  But I wasn't loading it in the test-then-ref order, I guess.  Thank you for checking on this!

I checked in that change.
(Assignee)

Updated

10 years ago
Depends on: 430652
(Assignee)

Comment 12

10 years ago
Created attachment 338692 [details] [diff] [review]
Fixed test

This is mochitest chrome variant of the reftest framework. I tested with rollback patch for this bug (expected failure) and also with patch for bug 295994 (expected success).
Attachment #338692 - Flags: review?(dbaron)
Attachment #338692 - Flags: review?(bzbarsky)
Comment on attachment 338692 [details] [diff] [review]
Fixed test

bzbarsky would be a better reviewer for this
Comment on attachment 338692 [details] [diff] [review]
Fixed test

> <html class="reftest-wait" style="height: 100%">

Ditch the class, and the className set below.

>+      ok(equal, "persistent attribute in tab box broken");
>+      
>+      dump("\n\n\n"+str1+"\n\n\n"+str2+"\n\n\n");

Put the URIs in the string for the ok() call instead of adding a separate dump.

With that, looks good.
Attachment #338692 - Flags: review?(dbaron)
Attachment #338692 - Flags: review?(bzbarsky)
Attachment #338692 - Flags: review+
(Assignee)

Comment 15

9 years ago
Created attachment 340540 [details] [diff] [review]
Fixed test2
[Checkin: Comment 16]
Attachment #338692 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Comment on attachment 340540 [details] [diff] [review]
Fixed test2
[Checkin: Comment 16]

http://hg.mozilla.org/mozilla-central/rev/cfe2d978cab1
Attachment #340540 - Attachment description: Fixed test2 → Fixed test2 [Checkin: Comment 16]
Attachment #338692 - Flags: review+
(In reply to comment #9)
> Checked in a reftest for this.

Which one ?
Assignee: nobody → honzab
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.1b1
The one that someone later went and removed.
Serge, for what it's worth the checkin comment on your checkin was wrong: you were just checking in a different test for this bug, not a fix for it.
Depends on: 623230
You need to log in before you can comment on or make changes to this bug.