Last Comment Bug 677658 - crash mozalloc_abort [@ mozalloc_abort(char const* const) | mozalloc_handle_oom() | nsHtml5TreeOperation::AppendText(wchar_t const*, int, nsIContent*, nsHtml5TreeOpExecutor*)]
: crash mozalloc_abort [@ mozalloc_abort(char const* const) | mozalloc_handle_o...
Status: RESOLVED FIXED
: crash, regression
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: x86 All
: -- critical (vote)
: mozilla8
Assigned To: Olli Pettay [:smaug] (high review load, please consider other reviewers)
:
Mentors:
http://forum.todae.fr/index.php?showt...
Depends on:
Blocks: 563322 677848
  Show dependency treegraph
 
Reported: 2011-08-09 12:57 PDT by Alice0775 White
Modified: 2011-09-01 14:51 PDT (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
backout+change to InsertAdjacentHTML (3.07 KB, patch)
2011-08-10 05:21 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review
patch (5.88 KB, patch)
2011-08-10 08:34 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
jonas: review+
Details | Diff | Review

Description Alice0775 White 2011-08-09 12:57:06 PDT
Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/f414db34c70b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110808 Firefox/8.0a1 ID:20110808030804

Reported @1for-matik
See http://forums.mozillazine.org/viewtopic.php?p=11111195#p11111195

High CPU and Huge memory usage while loading the page.
The browser become unresponsive.
And finally, the browser crashes with crash report.

bp-58beeda6-939e-48c8-855f-ccabe2110809 .


Reproducible: Always

Steps to Reproduce:

1. Start browser with clean profile
2. Open URL
3. 

Actual Results: 
  High CPU and Huge memory usage while loading the page.
  The browser become unresponsive.
  And finally, the browser crashes with crash report.

Expected Results: 
  No high CPU usage
  No Huge memory usage
  No Crash

Regression window(m-i hourly)
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2d6026252d03
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110731 Firefox/8.0a1 ID:20110731202744
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/7e84bd591246
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110731 Firefox/8.0a1 ID:20110801010116
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2d6026252d03&tochange=7e84bd591246

Suspected bug;
Bug 563322 - Does nsGenericHTMLElement::SetInnerHTML need to call scriptloader->SetEnabled(...)
Comment 1 Alice0775 White 2011-08-09 15:55:49 PDT
The following cset triggers the issue.
ff515cbd864e	Henri Sivonen — Bug 563322 part 1 - Avoid calling nsScriptLoader::SetEnabled in the innerHTML setter when using the HTML parser. r=Olli.Pettay.
Comment 2 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-10 04:44:36 PDT
I can certainly reproduce.
Henri is on vacation, so I can take a look.
Comment 3 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-10 05:18:26 PDT
Well, I can't reproduce the crash, but some kind of hang/endless loop.
Comment 4 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-10 05:21:41 PDT
Created attachment 552050 [details] [diff] [review]
backout+change to InsertAdjacentHTML

I uploaded this to tryserver.

But I still don't know why not-disabling scriptloader causes the problem
Comment 5 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-10 06:56:28 PDT
Ok, bug 563322 certainly regressed async script handling.
Comment 6 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-10 08:34:28 PDT
Created attachment 552084 [details] [diff] [review]
patch

Added nsAutoScriptBlockerSuppressNodeRemoved to fix unnecessary warning.
Includes also a test.

I think we should take this kind of approach for FF8, and then perhaps fix
this in other ways for FF9 (once hsivonen is back), so that disabling script
loader wouldn't be needed.

Uploaded the patch to tryserver.
Comment 7 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-10 08:36:09 PDT
And FYI, Webkit and Opera don't run the script when added using innerHTML.
Comment 8 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-10 11:14:11 PDT
Passed on try
Comment 9 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-10 13:35:47 PDT
Comment on attachment 552084 [details] [diff] [review]
patch

Review of attachment 552084 [details] [diff] [review]:
-----------------------------------------------------------------

Sucks to not know why this fixes the crash/hang, but this does seem like a safer approach for now.
Comment 10 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-10 14:04:18 PDT
http://hg.mozilla.org/mozilla-central/rev/3bf55a9e34e8
Comment 11 Henri Sivonen (:hsivonen) 2011-08-30 05:48:28 PDT
So this effectively undid bug 563322, right? What was the problem? Did the parser not mark scripts as already executed properly?
Comment 12 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-30 06:31:58 PDT
(In reply to Henri Sivonen (:hsivonen) from comment #11)
> So this effectively undid bug 563322, right?
Yes


> What was the problem? Did the
> parser not mark scripts as already executed properly?
IIRC nsScriptLoader::ProcessScriptElement just let the processing go through.
If scriptloader is disabled, that method returns early.

Note You need to log in before you can comment on or make changes to this bug.