25432 - Trees built dynamically inside a div cause a hang

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect, P2)

Description

[Michael La Guardia]

using 2000012715

Attempting to launch the cookie manager from the Task menu|My Wallet|Display
Cookies... freezes Seamonkey.  I have noticed this feature to be slow to launch
in the past, so this time I let it wait for about 15 minutes.  Still no response.

Comment 1

[Stephen P. Morse]

It sure does hang! :-(

Now to figure out who changed what in the ifrastructure that is causing this.

Comment 2

[Stephen P. Morse]

This seems to be very dependent on what is in the cookie file.  I have no idea 
yet what in the cookie file is causing the problem.

OK, after some more testing I discovered that things work fine if you have less 
than 8 cookies.  But if you have 8 or more cookies, you get this hang.

Comment 3

[Stephen P. Morse]

Ben, do you have time to take a look at this.  The cookie viewer is hanging on 
the eighth call to addItem from loadCookies.  The addItem routine is shown 
below.  The particular line in the addItem routine that hangs is at 
kids.appendChild at the end.  If I comment out the second setAttribute, the hang 
does not happen (although maybe then it will happen on the 16th call).

function AddItem(children,cells,prefix,idfier)
{
  var kids = document.getElementById(children);
  var item  = document.createElement("treeitem");
  var row   = document.createElement("treerow");
  for(var i = 0; i < cells.length; i++)
  {
    var cell  = document.createElement("treecell");
    cell.setAttribute("class", "propertylist");
    cell.setAttribute("value", cells[i])
    row.appendChild(cell);
  }
  item.appendChild(row);
  item.setAttribute("id",prefix + idfier);
  kids.appendChild(item);
}

Comment 4

[Stephen P. Morse]

I just said something wrong.  Remove the setAtribute does not stop the hang.  
But removing the appendChild does.

Comment 5

[Stephen P. Morse]

Guess what! -- 16 signons hang the singon viewer.

Comment 6

[Stephen P. Morse]

Let me clarify my clarification (one of these days I'll get it right).  We are 
hanging on the kids.appendChild at the end.  But if I remove the 
row.appendChild, then we do not hang on the kids.appendChild. 

Comment 7

[Stephen P. Morse]

Here's a big clue.  The cookie viewer can display up to 7 cookies without 
scrolling.  And it's when we add the 8th cookie to the list that we hang.

Furthermore, the signon viewer can hold many more than 7 signons before 
scrolling (the cookie viewer list was shortened to allow room for the properties 
at the bottom of the window).  Not sure how many, but it looks like 16 (which 
causes the hang) is beyond the limit.  Furthermore 11 does not cause a hang 
(above I said 15 was OK but I wasn't counting carefully then).

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

hyatt, you're the tree's bitch, what do you think about this?

Comment 9

[Suresh Duddi (gone)]

Beta1 this hang...

Comment 10

[Stephen P. Morse]

Reassigning to hyatt per ben's comment.

Comment 11

[Stephen P. Morse]

*** Bug 26212 has been marked as a duplicate of this bug. ***

Comment 12

[Michael La Guardia]

Does the tree widget hang in the same way anywhere other than Cookie Manager?  
If not, then I think that this is not a beta one bug, and Cookie Manager should 
be disabled if it can't be fixed on Steve's side.

Comment 13

[leger]

Putting on PDT+ radar for beta1.

Comment 14

[kevinyen]

Just echoing Michael's comments two sections above.

Per everyone's agreement on cookie manager being "in" for beta 1, if fixing this 
bug is needed only by cookie manager, then this bug should not be marked 
"beta1".

thx,
kevin

Comment 15

[David Hyatt]

PDT+??? Come on!

Protesting.

Comment 16

[leger]

Freeze is like a crash...crash is bad.  We would like fixed.  PDT+  This feature 
is in for beta1, you should not freeze.

Comment 17

[Stephen P. Morse]

Sorry for not replying to some of the questions above but after I reassigned 
this bug, I stopped getting e-mail notifications about it. :-(

As I mentioned above, it affects the single signon viewer as well as the cookie 
viewer.  It will also affect any other xul display that uses a similar list 
although offhand I don't know which they are.

Comment 18

[Michael La Guardia]

Freeze is like a crash.  Agreed.  Fixed in this case should then mean "disable 
the feature because the feature itself is not stop ship."  Changing summary to 
read "Cookie Manager freezes Seamonkey -- feature should be disabled for PR1"

Dave, I suggest that you reassign this bug to Steve.

Comment 19

[Suresh Duddi (gone)]

I think that would such a wrong thing to do. We shouldn't ship beta with the 
eighth call to appendChild() hanging. Maybe hyatt would know more.

Michael, please dont make technical decisions for us. I appreciate you thinking 
out of the box. But you are way out of line on this one.

Comment 20

[Suresh Duddi (gone)]

removing extra michaell from cc (he filed the bug).

Comment 21

[Michael La Guardia]

I agree that it is not my place to make technical decisions, but I don't think 
that I am attempting one here.

If cookie manager and single signon are the only places where this bug 
manifests itself, and I asked the question earlier and didn't receive any 
responses to the contrary, then I do not think that we need to fix this bug 
*now* so that cookie manager and single signon work.

I am making a *marketing* assertion that although I like both of these features, 
I would *not* hold the beta for them.

Comment 22

[Suresh Duddi (gone)]

I concur with your last stmt. That was the understanding all along. Nothing has 
changed that.

The bug is orthogonal to that. Regressions are orthogonal to that.

Comment 23

[Stephen P. Morse]

Symptoms appear to have changed slightly.  Before the hang was occuring as soon 
as you entered the cookie viewer.  Now it is occuring when you select a cookie 
after you have entered the viewer.  The number of cookies is still the limiting 
factor -- 7 or less and you don't get this hang, 8 or more and you do.

Comment 24

[Peter Trudelle]

<opinion>I agree with MichealL. If marketing would not hold beta for this
feature, then it should not be on the Beta1 list. We have too many problems with
core features to be able to prioritize things like this and still hit beta
target dates.  </opinion>

Setting priority P2, relative to hyatt's other open PDT+ bugs

Comment 25

[David Hyatt]

I have a fix for this.  Will check in when tree opens.

Comment 26

[Michael La Guardia]

Count on Dave to make all my ranting pointless!

I'm glad to hear of the fix.

Comment 27

[Michael La Guardia]

removing "should be disabled" from summary.

Comment 28

[Stephen P. Morse]

Well I'm glad to hear you have a fix too.  Even though I just spent a few hours 
coming up with a very simple test case.  Including the test case below:

CookieViewer.js
---------------

function Startup() {
  for(i = 0; i < 10; i++) {
    AddItem(["name"+i]); 
  }
  window.sizeToContent();
}

function ViewCookieSelected() {
  var field = document.getElementById("ifl_name");
  field.value = "hello";
}

function AddItem(cells) {
  var kids = document.getElementById("cookielist");
  var item  = document.createElement("treeitem");
  var row   = document.createElement("treerow");
  for(var i = 0; i < cells.length; i++) {
    var cell  = document.createElement("treecell");
    cell.setAttribute("class", "propertylist");
    cell.setAttribute("value", cells[i])
    row.appendChild(cell);
  }
  item.appendChild(row);
  kids.appendChild(item);
}

CookieViewer.xul
----------------

<?xml version="1.0"?>
<?xml-stylesheet href="chrome://wallet/skin/" type="text/css"?> 
<?xul-overlay href="chrome://global/content/dialogOverlay.xul"?>
<window id="cookieviewer"
        class="dialog"
        xmlns:html="http://www.w3.org/TR/REC-html40"
        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
        onload="Startup()">
  <html:script src="CookieViewer.js"/>
  <tabcontrol>
    <tabpanel>
      <box class="tabpanel" id="system">
        <html:div>
          <tree id="cookietree" class="inset" style="height: 150px"
            onselect="ViewCookieSelected();">
            <treecol width="100%"/>
            <treechildren id="cookielist"/>
          </tree>
        </html:div>
        <html:fieldset style="border: 2px groove #CCCCDD;">
          <html:legend></html:legend>
          <html:div>
            <html:input id="ifl_name" type="text" class="scroll-label"/>
          </html:div>
        </html:fieldset>
      </box>
    </tabpanel>
  </tabcontrol>
</window>

Comment 29

[Stephen P. Morse]

I forgot to mention that in the test case above, if you chang the "i<10" to be 
"i<5" the hang doesn't happen.  The hang occurs when you select any of the items 
in the list.

Comment 30

[David Hyatt]

Morphing this bug, since my fix isn't really solving the whole problem.  I've 
just made it much lower priority.

The cookie viewer has been patched to bypass the hang.

Comment 31

[Stephen P. Morse]

The patch that hyatt made in the cookie viewer doesn't solve the problem 
completly.  It does stop the hang on more than 7 cookies provided there are less 
than 15 cookie permissions (the other list in the tab of the cookie viewer).  If 
there are 15 or more cooke permissions, then you still hang when selecting a 
cookie regardless of how many cookies you have (e.g., I just demonstrated a hang 
with only 3 cookies).

And I'm sure we are still hanging on the signon viewer which has four tabs and 
all sorts of things that can go into it.

Comment 32

[Stephen P. Morse]

OK, I see the sort of change that hyatt made to the cookie tab of the signon 
viewer.  Making that same change to the cookie-permission tab fixes the problem 
I just reported.  I also made the same changes to the four tabs of the signon 
viewer.  I just checked in these changes so all my viewers are fixed.  Restoring 
summary to the way hyatt worded it -- namely "Trees built dynamically inside a 
div cause a hang".

Comment 33

[David Hyatt]

The fix for now is to remove the divs that wrap the trees.  Just let the trees 
be directly inside boxes.  You should be able to patch the other trees in your 
UI on your own.

Comment 34

[Nathan Kirkham]

Using WinNT SP5 Build 2000020708
I just hung the browser in Cookie viewer, has it been fixed ? I couldn't tell
from the comments.
If you think that it has I'll try and find what steps to reproduce.
Shouldn't this by set for beta1, you can't have known hangs killing the browser.

Comment 35

[David Hyatt]

Pulling back in and restoring PDT+ until we're sure we've licked everything.  OI 
am unable to make the cookie viewer hang.

Comment 36

[David Hyatt]

Steve, let's do this.  Make a new bug about the hang problem in WalletEditor.xul 
and assign it to Ben Goodger.  Ben can then go in and remove the trees from the 
XUL and keep the display right.  

I'm going to go ahead and remove the PDT+ and shove this bug out a ways now.

Comment 37

[David Hyatt]

I meant "remove the trees from the divs"... not from the XUL!  Oops.

Comment 38

[David Hyatt]

Fixed.

Comment 39

[Stephen P. Morse]

Something bad happened here.  On 2-7 dp inadvertently (I hope it was 
inadvertently) removed my name from the cc list so I haven't received any of the 
updates to this report since that time.  Just rediscovered it today.

So I never opened a separate bug report on WalletEditor.xul that hyatt asked me 
to do on 2-9.  But now that hyatt has fixed this bug, I guess there's no need 
for a separate WalletEditor bug report (or is there?).

Comment 40

[Tom Everingham]

verified:
Linux 2000042609
NT 2000042009