398289 - persist="selectedIndex" no longer working for tabbox elements

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Kaspar]

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".)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Can you create a testcase that's smaller than the entire cert manager, by any chance?  That would really help with getting this fixed...

Comment 2

[Kaspar]

Attachment: 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.

Comment 3

[Serge Gautherie (:sgautherie)]

[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 !?

Comment 4

[Serge Gautherie (:sgautherie)]

[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
]

Comment 5

[Kaspar]

(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...? ;-)

Comment 6

[Boris Zbarsky [:bzbarsky]]

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?

Comment 7

[Kaspar]

(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!

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So it sounds like this is fixed? If not, feel free to renominate.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Checked in a reftest for this.

Comment 10

[Kaspar]

(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)?

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: 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).

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 338692 [details] [diff] [review]
Fixed test

bzbarsky would be a better reviewer for this

Comment 14

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

[Honza Bambas (:mayhemer)]

Attachment: Fixed test2 [Checkin: Comment 16]

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 340540 [details] [diff] [review]
Fixed test2
[Checkin: Comment 16]

http://hg.mozilla.org/mozilla-central/rev/cfe2d978cab1

Comment 17

[Serge Gautherie (:sgautherie)]

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

Which one ?

Comment 18

[Boris Zbarsky [:bzbarsky]]

The one that someone later went and removed.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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.