Closed Bug 220603 Opened 21 years ago Closed 21 years ago

The above URL causes Mozilla to peg the CPU and hang


(Core :: DOM: Core & HTML, defect)

Not set





(Reporter: alther, Assigned: jst)





(2 files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030624
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030624

Mozilla partially loads, but then
pegs the CPU and becomes unresponsive (i.e. hangs).

Reproducible: Always

Steps to Reproduce:
1. Enter into the location bar
and press 'Enter'

Actual Results:  
Page partially loads then 'hangs'.  Checking the task manager shows Mozilla
consuming all CPU cycles.

Expected Results:  
Page to render properly.
Actually, it seems that all IMDB photogallery URLs are causing this problem with
Mozilla.  They must have changed something recently and I'm sure it was working
with 1.4 before.
worksforme 2003092604/win98SE
The site loads an ad script that loads

This latter script scrapes all the content on the page and phones home to the ad
server with it (eeeew).

The problem is that the code it uses to scrape is very very slow.  Glacially.

The biggest culprits actually seem to be JS engine overhead and security manager
access (getting object principals)....

The server does a lot of UA sniffing (eg it returns a 403 for wget), so I wonder
what content they serve to IE....
Depends on: 213946
confirming on winxp
Ever confirmed: true
*** Bug 221095 has been marked as a duplicate of this bug. ***
Confirming on Linux build 2003100205
H/OS -> All/All
OS: Windows 2000 → All
Hardware: PC → All
Blocks: 221236
So here is the skinny:  This bug is only reproducible if you have enabled strict
JS warnings.  The page loads a script (show_scraped_ads.js) that does the
following, more or less (irrelevant stuff excised):

  function google_show_ad() {
    document.write("<script src='scrape.js'><" + "/script>");
  function google_error_handler(message, url, line) {
    return true;
  window.onerror = google_error_handler;
  // more stuff

And scrape.js has the following code:

  var google_old_onload = window.onload;
  function google_ad() {
    // walk the DOM some
    if (google_old_onload) google_old_onload();
  window.onload = google_ad;

Now when strict warnings are enabled, warnings trigger the onerror handler (see
bug 63672).  But unlike real errors, warnings do _not_ stop script execution. 
So google_show_ad is called at least twice if there is a strict warning
somewhere on the page (which there is -- assigning to undeclared var).  So
scrape.js is executed twice.  Note that if this happens, the following holds:

   window.onload == google_old_onload == google_ad

So now onload google_ad is called... and calls itself recursively and goes into
an infinite loop (since it does not unset google_old_onload after calling it).

So the only thing mozilla does at all questionably is that strict warnings
trigger onerror but do not stop script execution -- this ad script depends on
the fact that onerror means script execution will be halted.  Note that a real
error somewhere in scrape.js after the onload-munging code would get the script
in the exact same infinite loop in all browsers...

Brendan?  jst?  Thoughts?
The bug is that onerror should not be called for warnings.  That's not backward
compatible with Nav[234] of sainted memory, because as bz notes, warnings do not
stop script execution.

BTW, the true return from the scripted onerror just says "I've reported the
error, no need for a standard DOM report."

Assignee: general → jst
Component: Browser-General → DOM Level 0
Brendan, see comments in bug bug 63672.  It seems that the onerror behavior is
somewhat intentional... (not to mention that that bug has a possible patch if
the behavior needs changing).
bz: that bug mixes up two options, strict and werror.  The latter makes warnings
be errors.  The right test in the patch is therefore

+      if (docShell &&
+          (!JSREPORT_IS_STRICT(report->flags) ||
+           JSREPORT_IS_WERROR(report->flags))) {


OK.  sr=me for that change...  I guess jst needs to r=?
Er, I mean:

+      if (docShell && JSREPORT_IS_WARNING(report->flags)) {

If you have the werror option set, warnings become errors and JSREPORT_WARNING
will be cleared from report->flags.  You can have strict warnings or strict
errors, so the above test shouldn't test for STRICT at all.  The only question
is whether the current report is a warning or an error (whether hard or mapped
from a warning due to werror).

Should we take this up in bug 63672, reopened?  Prolly not at this late date.


Agh, I mean:

+      if (docShell && !JSREPORT_IS_WARNING(report->flags)) {

All clear?

Heh.  Yeah, that's better. And no, it doesn't make much sense to take this up in
bug 63672 now, though we will need to reopen it and mark it duplicate, probably....
Comment on attachment 133384 [details] [diff] [review]
Brendan's patch

sr=bzbarsky.  jst, could you please review so we can get this bug fixed or
Attachment #133384 - Flags: superreview+
Attachment #133384 - Flags: review?(jst)
Comment on attachment 133384 [details] [diff] [review]
Brendan's patch

Attachment #133384 - Flags: review?(jst) → review+
Checked in.
Closed: 21 years ago
Resolution: --- → FIXED
No longer blocks: 221236
*** Bug 63672 has been marked as a duplicate of this bug. ***
*** Bug 221236 has been marked as a duplicate of this bug. ***
I think this patch may have also fixed bug 104549.
Verified FIXED using 2004-07-25-09 on Windows XP.
You need to log in before you can comment on or make changes to this bug.