Closed Bug 25432 Opened 25 years ago Closed 25 years ago

Trees built dynamically inside a div cause a hang

Categories

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

Product:
Core
Component:
Networking: Cookies
Platform:
x86
Other
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: michaell, Assigned: hyatt)

References

Details

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.
It sure does hang! :-(

Now to figure out who changed what in the ifrastructure that is causing this.
Status: NEW → ASSIGNED
Target Milestone: M14
Blocks: 25154
Summary: Cookie Manager freezes Seamonkey → [regression] Cookie Manager freezes Seamonkey
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.
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);
}
I just said something wrong.  Remove the setAtribute does not stop the hang.  
But removing the appendChild does.
Guess what! -- 16 signons hang the singon viewer.
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. 
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).
hyatt, you're the tree's bitch, what do you think about this?
Beta1 this hang...
Keywords: beta1
Reassigning to hyatt per ben's comment.
Assignee: morse → hyatt
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
*** Bug 26212 has been marked as a duplicate of this bug. ***
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.

Putting on PDT+ radar for beta1.
Whiteboard: [PDT+]
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
PDT+??? Come on!

Protesting.
Whiteboard: [PDT+] → [PDT]
Whiteboard: [PDT]
Freeze is like a crash...crash is bad.  We would like fixed.  PDT+  This feature 
is in for beta1, you should not freeze.
Whiteboard: [PDT+]
Keywords: regression
Summary: [regression] Cookie Manager freezes Seamonkey → Cookie Manager freezes Seamonkey
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.
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.
Summary: Cookie Manager freezes Seamonkey → Cookie Manager freezes Seamonkey -- feature should be disabled for PR1
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.
removing extra michaell from cc (he filed the bug).
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.
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.
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.
<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
Priority: P3 → P2
I have a fix for this.  Will check in when tree opens.

Count on Dave to make all my ranting pointless!

I'm glad to hear of the fix.
removing "should be disabled" from summary.
Summary: Cookie Manager freezes Seamonkey -- feature should be disabled for PR1 → Cookie Manager freezes Seamonkey
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>

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.
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.
Keywords: beta1, regression
Summary: Cookie Manager freezes Seamonkey → Trees built dynamically inside a div cause a hang
Whiteboard: [PDT+]
Target Milestone: M14 → M16
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.
Summary: Trees built dynamically inside a div cause a hang → Hang in cookie viewer and signon viewer if too many items in list
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".
Summary: Hang in cookie viewer and signon viewer if too many items in list → Trees built dynamically inside a div cause a hang
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.

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.

Pulling back in and restoring PDT+ until we're sure we've licked everything.  OI 
am unable to make the cookie viewer hang.
Whiteboard: [PDT+]
Target Milestone: M16 → M14
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.
Whiteboard: [PDT+]
Target Milestone: M14 → M16
I meant "remove the trees from the divs"... not from the XUL!  Oops.

Fixed.
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
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?).
verified:
Linux 2000042609
NT 2000042009
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.