220603 - The above URL causes Mozilla to peg the CPU and hang

Status: VERIFIED FIXED

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

Description

[Rick Alther]

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 http://us.imdb.com/name/nm0000205/photogallery, but then
pegs the CPU and becomes unresponsive (i.e. hangs).

Reproducible: Always

Steps to Reproduce:
1. Enter http://us.imdb.com/name/nm0000205/photogallery 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.

Comment 1

[Rick Alther]

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.

Comment 2

[Daniel Wang]

worksforme 2003092604/win98SE

Comment 3

[Boris Zbarsky [:bzbarsky]]

The site loads an ad script that loads
http://pagead2.googlesyndication.com/pagead/scrape.js

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....

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: gzipped jprof profile of letting the CPU spin for a while

Comment 5

[Markus Hübner]

confirming on winxp

Comment 6

[Charles Fenwick]

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

Comment 7

[Max Alekseyev]

Confirming on Linux build 2003100205
H/OS -> All/All

Comment 8

[Boris Zbarsky [:bzbarsky]]

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) {
    google_show_ad();
    return true;
  }
  window.onerror = google_error_handler;
  // more stuff
  google_show_ad();

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?

Comment 9

[Brendan Eich [:brendan]]

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."

/be

Comment 10

[Boris Zbarsky [:bzbarsky]]

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).

Comment 11

[Brendan Eich [:brendan]]

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))) {

/be

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK.  sr=me for that change...  I guess jst needs to r=?

Comment 13

[Brendan Eich [:brendan]]

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.

/be

Comment 14

[Brendan Eich [:brendan]]

Agh, I mean:

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

All clear?

/be

Comment 15

[Boris Zbarsky [:bzbarsky]]

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 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Brendan's patch

Comment 17

[Boris Zbarsky [:bzbarsky]]

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
something?

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 133384 [details] [diff] [review]
Brendan's patch

r=jst

Comment 19

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

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

Comment 22

[Christopher Aillon (sabbatical, not receiving bugmail)]

I think this patch may have also fixed bug 104549.

Comment 23

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using 2004-07-25-09 on Windows XP.