ICEfaces
  1. ICEfaces
  2. ICE-9841

"Stop running script" alert displayed in IE8 (revert back to jQuery 1.7)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: EE-3.3.0.GA_P01
    • Fix Version/s: EE-3.3.0.GA_P02
    • Component/s: ACE-Components, Framework
    • Labels:
      None
    • Environment:
      ICEfaces-EE-3.3.0.GA_P01-Williams-tableConfigEnh-Build-02102014
    • Assignee Priority:
      P1
    • Salesforce Case Reference:

      Description

      With the recent W*****s patched builds they are seeing a "Stop running this script?" message in IE8. This is reproducible with a provided POC war file in IE8.

      Steps to reproduce:
       - Load Screen 1 in the first tab.
       - Click the Retrieve button, table loads without any issue.
       - Click the Retrieve button again, the message is shown.

      **EDIT**

      The performance issues can also be seen when setting DEFAULT_LIST_SIZE to 200 in DataTableData.java in the showcase and loading the ace:dataTable > Scrolling demo and selecting the last row, which takes a while to be selected.

        Activity

        Hide
        Arran Mccullough added a comment -

        War file has been uploaded to the iceads1\Users\arran\ICE-9841

        Show
        Arran Mccullough added a comment - War file has been uploaded to the iceads1\Users\arran\ ICE-9841
        Hide
        Arturo Zambrano added a comment -

        This situation seems to be caused only by rendering performance issues of the IE8 browser. The problem only occurs on a real IE8 and not on IE10 or newer in IE8 mode.

        I also found out that this is relative to the number of rows on the table. The 'screen 1' view in the POC has a table with 50 rows per page. If the number of rows is changed to 45 or less, the message doesn't appear (at least in icepcvm-ie8).

        I also confirmed that the message is not caused by executing showProcessingMessage('Retrieving') on the POC, which shows a message while loading.

        Show
        Arturo Zambrano added a comment - This situation seems to be caused only by rendering performance issues of the IE8 browser. The problem only occurs on a real IE8 and not on IE10 or newer in IE8 mode. I also found out that this is relative to the number of rows on the table. The 'screen 1' view in the POC has a table with 50 rows per page. If the number of rows is changed to 45 or less, the message doesn't appear (at least in icepcvm-ie8). I also confirmed that the message is not caused by executing showProcessingMessage('Retrieving') on the POC, which shows a message while loading.
        Hide
        Arturo Zambrano added a comment -

        After some more investigation, I've discovered more details. There doesn't seem to be a single code point that's causing the increased processing time.

        First, I've confirmed that the problem is not in the resizeScrolling() function. After measuring the processing time of this function in milliseconds, it's always around 300 ms. Moreover, after adding some logging and testing several times, I could see that the slow processing happens BEFORE this function in invoked. This function in invoked only once (for the outer table).

        When clicking on the 'Retrieve' button, the whole table markup is updated, and thus the whole table and its contents are re-initialized. The increased processing time seems to be caused by the inner tables and calendars that have to be initialized every time the outer table is refreshed. For, example, if we remove the inner tables from the screen1.xhtml document, we can even increase the number of rows to 200, and we never see that browser message to stop running the script.

        I replaced these inner tables by standard h:dataTables, and I couldn't see the message asking to stop the script. I even increased the number of rows 200, and didn't see it either. So, the problem seems to be with initializing these inner tables, and that's where the focus of investigation will be next.

        I just wonder if this is a newly introduced issue or if it was possible to reproduce it with previous builds? Was it just discovered now and could've been discovered earlier or not?

        Show
        Arturo Zambrano added a comment - After some more investigation, I've discovered more details. There doesn't seem to be a single code point that's causing the increased processing time. First, I've confirmed that the problem is not in the resizeScrolling() function. After measuring the processing time of this function in milliseconds, it's always around 300 ms. Moreover, after adding some logging and testing several times, I could see that the slow processing happens BEFORE this function in invoked. This function in invoked only once (for the outer table). When clicking on the 'Retrieve' button, the whole table markup is updated, and thus the whole table and its contents are re-initialized. The increased processing time seems to be caused by the inner tables and calendars that have to be initialized every time the outer table is refreshed. For, example, if we remove the inner tables from the screen1.xhtml document, we can even increase the number of rows to 200, and we never see that browser message to stop running the script. I replaced these inner tables by standard h:dataTables, and I couldn't see the message asking to stop the script. I even increased the number of rows 200, and didn't see it either. So, the problem seems to be with initializing these inner tables, and that's where the focus of investigation will be next. I just wonder if this is a newly introduced issue or if it was possible to reproduce it with previous builds? Was it just discovered now and could've been discovered earlier or not?
        Hide
        Arturo Zambrano added a comment -

        The performance issue seems proportional to the number of cells in the inner tables. If we simply remove one column from the inner table, we no longer see the warning message. However, if we increase the number of rows in the table, we see the warning message again.

        Show
        Arturo Zambrano added a comment - The performance issue seems proportional to the number of cells in the inner tables. If we simply remove one column from the inner table, we no longer see the warning message. However, if we increase the number of rows in the table, we see the warning message again.
        Hide
        Arturo Zambrano added a comment -

        The dataTable initialization function calls several other functions to set up different features and functionality. Removing the sorting and selection setup functions, improved the situation, since the warning message didn't appear. Removing all the other setup functions, didn't improve the situation. So, it seems like the bulk of the problem is in the sorting and selection setup. It possibly has to do with jQuery selectors.

        Show
        Arturo Zambrano added a comment - The dataTable initialization function calls several other functions to set up different features and functionality. Removing the sorting and selection setup functions, improved the situation, since the warning message didn't appear. Removing all the other setup functions, didn't improve the situation. So, it seems like the bulk of the problem is in the sorting and selection setup. It possibly has to do with jQuery selectors.
        Hide
        Arturo Zambrano added a comment -

        Further testing revealed that the issue is reproducible with the build from November the first, 2013. Most likely, general changes related to ICE-9471 caused this situation.

        There doesn't seem to be a single point of failure. I simplified the jQuery selector used to access the row, and it improved things, but if the number of rows was increased, the stop script message appeared again.

        Show
        Arturo Zambrano added a comment - Further testing revealed that the issue is reproducible with the build from November the first, 2013. Most likely, general changes related to ICE-9471 caused this situation. There doesn't seem to be a single point of failure. I simplified the jQuery selector used to access the row, and it improved things, but if the number of rows was increased, the stop script message appeared again.
        Hide
        Arturo Zambrano added a comment -

        I reverted to revision 38615 in the tableConfigEnh branch, which is the very revision when that branch was created, and I tried to reproduce the issue with that code. The issue was still reproducible in the same way. So, it seems that this performance issue on ie8 is not related to the work related to ICE-9471, but to earlier fixes targeted for ICEfaces 3.3.

        Show
        Arturo Zambrano added a comment - I reverted to revision 38615 in the tableConfigEnh branch, which is the very revision when that branch was created, and I tried to reproduce the issue with that code. The issue was still reproducible in the same way. So, it seems that this performance issue on ie8 is not related to the work related to ICE-9471 , but to earlier fixes targeted for ICEfaces 3.3.
        Hide
        Arturo Zambrano added a comment -

        Using the 3.2 release jars and testing on the POC, I could see that there was no issue and the rows are retrieved fairly fast.

        Using the 3.3 release, I could see that retrieving the rows actually takes longer than it takes now, but the stop script message doesn't appear.

        Show
        Arturo Zambrano added a comment - Using the 3.2 release jars and testing on the POC, I could see that there was no issue and the rows are retrieved fairly fast. Using the 3.3 release, I could see that retrieving the rows actually takes longer than it takes now, but the stop script message doesn't appear.
        Hide
        Arturo Zambrano added a comment -

        After doing an extensive binary search of previous revisions, it was found that the stop script error message started to appear after the ICE-9274 fix in May of last year. However, in revisions just before that fix, the performance on IE8 was just as bad. That fix simply made the problem more evident. Eventually, the approach of ICE-9274 was replaced by a better approach in ICE-9520.

        I went further back in revisions and noticed a dramatic degradation in performance in IE8 right after upgrading to jQuery 1.9.1 (ICE-8920) in February of last year. The performance degradation was not only caused by the new jQuery version, but by some changes in the data table javascript. At the end, the problem was caused by the use of the jQuery.find(selector) function, which is odd, since jQuery.find() is supposed to have better performance.

        In the end, what solved the performance issues in the Williams branch was 1) applying ICE-9520 to it, 2) reverting to the previous version of jQuery (1.7), and 3) changing the occurrences of jQuery.find(selector) for ice.ace.jq(selector) and moving the selectors from the prototype to object properties.

        Show
        Arturo Zambrano added a comment - After doing an extensive binary search of previous revisions, it was found that the stop script error message started to appear after the ICE-9274 fix in May of last year. However, in revisions just before that fix, the performance on IE8 was just as bad. That fix simply made the problem more evident. Eventually, the approach of ICE-9274 was replaced by a better approach in ICE-9520. I went further back in revisions and noticed a dramatic degradation in performance in IE8 right after upgrading to jQuery 1.9.1 ( ICE-8920 ) in February of last year. The performance degradation was not only caused by the new jQuery version, but by some changes in the data table javascript. At the end, the problem was caused by the use of the jQuery.find(selector) function, which is odd, since jQuery.find() is supposed to have better performance. In the end, what solved the performance issues in the Williams branch was 1) applying ICE-9520 to it, 2) reverting to the previous version of jQuery (1.7), and 3) changing the occurrences of jQuery.find(selector) for ice.ace.jq(selector) and moving the selectors from the prototype to object properties.
        Hide
        Arturo Zambrano added a comment -

        Backported fix to 3.3 EE branch at revision 40412.

        Reverting to jQuery 1.7 in the 3.3 EE branch was desirable and feasible, since it improves dramatically performance on IE8 browsers, without affecting the performance of other browsers. Moreover, QA reports that the failures seen after reverting to jQuery 1.7 in the customer branch, were fixed by simply updating the selenium scripts. So, no new real bugs were introduced by this change.

        Show
        Arturo Zambrano added a comment - Backported fix to 3.3 EE branch at revision 40412. Reverting to jQuery 1.7 in the 3.3 EE branch was desirable and feasible, since it improves dramatically performance on IE8 browsers, without affecting the performance of other browsers. Moreover, QA reports that the failures seen after reverting to jQuery 1.7 in the customer branch, were fixed by simply updating the selenium scripts. So, no new real bugs were introduced by this change.

          People

          • Assignee:
            Arturo Zambrano
            Reporter:
            Arran Mccullough
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: