100% cpu in any view, no events (workaround 1.7/1.8 getElementsByAttribute incompatibility)

RESOLVED FIXED

Status

Calendar
Sunbird Only
RESOLVED FIXED
14 years ago
12 years ago

People

(Reporter: gekacheka, Assigned: Mostafa Hosseini)

Tracking

Details

Attachments

(7 attachments, 3 obsolete attachments)

1.39 KB, patch
Mostafa Hosseini
: first-review+
Details | Diff | Splinter Review
2.30 KB, patch
Mostafa Hosseini
: first-review+
Details | Diff | Splinter Review
2.38 KB, patch
Mostafa Hosseini
: first-review+
Details | Diff | Splinter Review
2.30 KB, patch
Mostafa Hosseini
: first-review+
Details | Diff | Splinter Review
1.98 KB, patch
Mostafa Hosseini
: first-review+
Details | Diff | Splinter Review
1.83 KB, patch
Mostafa Hosseini
: first-review+
Details | Diff | Splinter Review
1.00 KB, patch
Mostafa Hosseini
: first-review+
Details | Diff | Splinter Review
(Reporter)

Description

14 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040421
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040421

Switching to any of the views hangs with 100% cpu when running calendar code
from cvs on Moz1.7.  No events are displayed in grids.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.




Problem is due to changes made for Bug 240186, which changes the behavior of
document.getElementsByAttribute.  The result in 1.8 is a 'live' list, so adding
or removing elements of the list while traversing the list changes the index of
all elements after it.  The changes made to accommodate the new 'live' behavior
are not compatible with the old behavior.

In particular, many places where the code did something like

  var selectedBoxes = getElementsByAttribute("selected","true");
  for (var i = 0; i < selectedBoxes.length; i++)
     selectedBoxes[i].parentNode.removeChild(selectedBoxes[i]);

were changed to something like

  var selectedBoxes = getElementsByAttribute("selected","true");
  while(selectedBoxes.item(0)) {
     var box = selectedBoxes.item(0);
     box.parentNode.removeChild(box);
  }
Note removing the node from its parent also removes it from the 'live' list.

Similarly, code that did something like

  var markedBoxes = document.getElementByAttribute("selected","true");
  for (var i = 0; i < markedBoxes.length; i++) 
     markesBoxes[i].removeAttribute("selected");

was changed to 

  var markedBoxes = document.getElementByAttribute("selected","true");
  while(markedBoxes.item(0)) {
     markedBoxes.item(0).removeAttribute("selected"):
  }

Note changing the attribute also removes it from the 'live' list.

But the new 'live' behavior of the lists returned by getElementsByAttribute is
incompatible with the old 'dead' behavior. 

A workaround that should work with both the old 1.7 behavior and the new 1.8
behavior is to copy the list into a temporary array first, and then delete the
elements of the array.

  var liveList = document.getElementByAttribute("selected","true");
  var deadArray = new Array(liveList.length);
  for (var i = 0; i < deadArray.length; i++)
     deadArray[i] = liveList.item(i);
  liveList = null;
  for (var i = 0; i < deadArray.length; i++)
     deadArray[i].removeAttribute("selected"):
(Reporter)

Comment 1

14 years ago
Created attachment 147955 [details] [diff] [review]
Patch calendarWindow.js (base class methods)

Adds CalendarView utility methods called by all derived views.
  removeElementsByAttribute
  removeAttributeFromElements
  getArrayOfElementsByAttribute (called by above 2)
(Reporter)

Comment 2

14 years ago
Created attachment 147956 [details] [diff] [review]
dayView.js patch: call calendarView methods

Patch dayView to call calendarView methods.
   removeElementsByAttribute
   removeAttributeFromElements
(Reporter)

Comment 3

14 years ago
Created attachment 147957 [details] [diff] [review]
weekView.js patch: call calendarView methods

Patch weekView.js to call calendarView methods
  removeElementsByAttribute
  removeAttributeFromElements
(Reporter)

Comment 4

14 years ago
Created attachment 147959 [details] [diff] [review]
multiweekView.js patch: call calendarView methods

Patch week view to call calendar view methods
  removeElementsByAttribute
  removeAttributeFromElements
(Reporter)

Comment 5

14 years ago
Created attachment 147960 [details] [diff] [review]
monthView.js patch: call calendarView methods

Patch monthView.js to call calendarView methods
  removeElementsByAttribute
  removeAttributeFromElements
(Reporter)

Comment 6

14 years ago
Created attachment 147961 [details] [diff] [review]
dragDrop.js: copy result of getElementsByAttribute before removing same attribute "draggedover"
(Reporter)

Comment 7

14 years ago
Created attachment 147962 [details] [diff] [review]
unifinderTodo.js: copy result of getElementsByAttribute before removing same attribute "checked"
(Reporter)

Updated

14 years ago
Attachment #147955 - Flags: first-review+
(Reporter)

Updated

14 years ago
Attachment #147956 - Flags: first-review+
(Reporter)

Updated

14 years ago
Attachment #147955 - Flags: first-review+ → first-review?(mostafah)
(Reporter)

Updated

14 years ago
Attachment #147956 - Flags: first-review+ → first-review?(mostafah)
(Reporter)

Updated

14 years ago
Attachment #147957 - Flags: first-review?(mostafah)
(Reporter)

Updated

14 years ago
Attachment #147959 - Flags: first-review?(mostafah)
(Reporter)

Updated

14 years ago
Attachment #147960 - Flags: first-review?(mostafah)
(Reporter)

Updated

14 years ago
Attachment #147961 - Flags: first-review?(mostafah)
(Reporter)

Updated

14 years ago
Attachment #147962 - Flags: first-review?(mostafah)
(Reporter)

Updated

14 years ago
Depends on: 240186
OS: Windows 2000 → All
Hardware: PC → All
Or you could walk the list in reverse order and delete elements -- that would
work with both the new and old behavior, no?

That said, that's much slower than the code I wrote (which is why I wrote the
code as I did); I was not aware that current calendar XPIs had the requirement
to be installable on old builds....
(Reporter)

Comment 9

14 years ago
Created attachment 147982 [details] [diff] [review]
calendarWindow.js patch (base class methods)

Updated to remove in reverse order, avoiding copy.
Attachment #147955 - Attachment is obsolete: true
(Reporter)

Comment 10

14 years ago
Created attachment 147983 [details] [diff] [review]
dragDrop.js patch (remove attributes in reverse order)

Removes in reverse order, avoiding copy.
(Reporter)

Updated

14 years ago
Attachment #147961 - Attachment is obsolete: true
(Reporter)

Comment 11

14 years ago
Created attachment 147984 [details] [diff] [review]
unifinderTodo.patch (remove in reverse order)

Remove in reverse order, avoid copy.
Attachment #147962 - Attachment is obsolete: true
(Reporter)

Updated

14 years ago
Attachment #147982 - Flags: first-review?(mostafah)
(Reporter)

Updated

14 years ago
Attachment #147983 - Attachment description: dragDrop.js patch (remove elements in reverse order) → dragDrop.js patch (remove attributes in reverse order)
Attachment #147983 - Flags: first-review?(mostafah)
(Reporter)

Updated

14 years ago
Attachment #147984 - Flags: first-review?(mostafah)
(Reporter)

Updated

14 years ago
Attachment #147955 - Flags: first-review?(mostafah)
(Reporter)

Updated

14 years ago
Attachment #147961 - Flags: first-review?(mostafah)
(Reporter)

Updated

14 years ago
Attachment #147962 - Flags: first-review?(mostafah)
(Assignee)

Updated

14 years ago
Attachment #147982 - Flags: first-review?(mostafah) → first-review+
(Assignee)

Updated

14 years ago
Attachment #147956 - Flags: first-review?(mostafah) → first-review+
(Assignee)

Updated

14 years ago
Attachment #147957 - Flags: first-review?(mostafah) → first-review+
(Assignee)

Updated

14 years ago
Attachment #147959 - Flags: first-review?(mostafah) → first-review+
(Assignee)

Updated

14 years ago
Attachment #147960 - Flags: first-review?(mostafah) → first-review+
(Assignee)

Updated

14 years ago
Attachment #147983 - Flags: first-review?(mostafah) → first-review+
(Assignee)

Updated

14 years ago
Attachment #147984 - Flags: first-review?(mostafah) → first-review+
(Assignee)

Comment 12

14 years ago
All reviewed patches checked into CVS.
Problem fixed and the code looks nicer.
Thanks a lot.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
The bugspam monkeys have been set free and are feeding on Calendar :: Sunbird Only. Be afraid for your sanity!
QA Contact: gurganbl → sunbird
You need to log in before you can comment on or make changes to this bug.