In Firefox 2.0.0.1, clicking on the title of a talkback on ynet.co.il does not open the body of the talkback. Instead, the error console shows "showTb is not defined". This didn't happen in Fx 2.0, nor does it happen in the latest trunk build. Steps to reproduce: 1. Go to the URL above. 2. Scroll down to the talkbacks section (the numbered list below the blue bar). 3. Click on one of the items. 4. Nothing happens, but the error console shows an error. I'll attach a reduced testcase soon.

This testcase should show two alerts, but only the first one is shown in Fx 2.0.0.1. What happens here is: 1. Script embedded in the body dynamically inserts a <script> tag into the head. 2. The script inserted into the head alters the innerHtml of an element in the body. 3. As a result, another script element in the body is ignored (in the original page, this was the element that contained the showTb function definition).

Requesting blocking1.8.1.2, since this is a regression affecting major functionality on one of the most popular Israeli sites.

Regression range is 2006-11-10-07 to 2006-11-11-07: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-11-10+06%3A00&maxdate=2006-11-11+08%3A00&cvsroot=%2Fcvsroot My (uneducated) guesses for possible candidates are either bug 336731, or a check-in not specifying a bug number, with the comment "Don't execute scripts from BindToTree if a mutation is in progress. r/sr=bz a=dveditz".

This regressed in 1.5.0.9 as well, with a similar regression range on the 1.8.0 branch (2006-11-10-03 to 2006-11-11-03): http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_0_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-11-10+02%3A00&maxdate=2006-11-11+04%3A00&cvsroot=%2Fcvsroot I guess this takes bug 336731 off the hook.

Blake: Can you look into this regression from your patch in bug 336731? Uri: We landed that fix on both branches around the same time, so it still could potentially be the cause.

This could very well be a regression from the BindToTree patch, i.e. bug 343730. I'll look into it on monday.

(In reply to comment #5) > Blake: Can you look into this regression from your patch in bug 336731? Jay, given that the testcase here doesn't use Object.prototype.watch, I don't think that it could be the cause of this regression. Bug 343730 seems like a much more likely candidate, though if anybody has time to check via local backout, that would be ideal.

I confirmed that backing the patch from bug 343730 out of my branch tree gives me two alerts on the testcase.

Ok, here's what happens: The document looks something like this: <script id=1> ..create and insert a new script element (id=2) using DOM methods the new script has non-inline code that sets innerHTML on a random node.. </script> <script id=3> ...do stuff... </script> <script id=4> ...do stuff... </script> When script 1 is parsed it executes immediately which creates and inserts script 2 into the DOM. The scriptloader is notified about script 2 but since it's non-inline it's not executed right away but rather put on the request queue. Script 3 is parsed but since the request queue is not empty it is added to the end of the queue and the parser is blocked and we return to the event loop. Script 2 finishes loading and sets innerHTML. Setting innerHTML first disables the scriptloader, then modifies the DOM thereby calling BeginUpdate/EndUpdate The EndUpdate calls RemoveBlocker which makes us start processing the request queue. Script 3 executes Since Script 3 was the last queued script we resume parsing (this is still while we're inside SetInnerHTML) Script 4 is parsed, but since the scriptloader is still disabled script 4 is ignored. Parsing finishes, we return back up the callstack to SetInnerHTML where the scriptloader is enabled again. So there are at least two things that are bad here: 1. We shouldn't execute script 3 while inside SetInnerHTML. 2. We shouldn't execute script 3 while the scriptloader is disabled 1 is fixed by making RemoveBlocker not execute scripts synchronously but rather off an event. 2 is fixed by adding an mEnabled test to ProcessPendingRequests. However that probably means that we in nsScriptLoader::SenEnabled might need to post an event to process pending scripts. Not sure if 2 needs to be done on branch though since I can't actually think of a way this could happen once 1 is fixed. Need to investigate that.

This should fix it for the branch. I didn't want to add the mEnabled check to the branch since it hasn't been there before.

This testcase is slightly safer and doesn't mess up debugging by firing alerts and such. It also tests that all scripts execute when they should.

Btw, I love the new events stuff on trunk. This is the same as the branch patch except that it also makes us not process pending requests when mEnabled is false. Which also requires that we start processing pending events once the scriptloader is reenabled.

Landed the trunk patch.

on balsa: c++ -I/usr/X11R6/include -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-long-long -pedantic -fshort-wchar -pthread -pipe -DDEBUG -D_DEBUG -DDEBUG_cltbld -DTRACING -g -fno-inline -fPIC -shared -Wl,-z,defs -Wl,-h,libtoolkitcomps.so -o libtoolkitcomps.so nsToolkitCompsModule.o -lpthread -Wl,--whole-archive ../startup/src/libappstartup_s.a ../downloads/src/libdownload_s.a ../alerts/src/libalerts_s.a ../../../dist/lib/liburlclassifier_s.a ../../../dist/lib/libfeed_s.a ../typeaheadfind/src/libfastfind_s.a -Wl,--no-whole-archive -L../../../dist/bin -L../../../dist/lib -lgkgfx ../../../dist/lib/libunicharutil_s.a -L../../../dist/bin -lxpcom -lxpcom_core -L../../../dist/bin -L../../../dist/lib -lplds4 -lplc4 -lnspr4 -lpthread -ldl -L../../../dist/bin -lmozjs -Wl,-Bsymbolic -ldl -lm ../../../dist/lib/libfeed_s.a(nsScriptableUnescapeHTML.o): In function `nsScriptLoader::SetEnabled(int)': /builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/toolkit/components/feeds/src/../../../../dist/include/xpcom/nsVoidArray.h:64: undefined reference to `nsScriptLoader::ProcessPendingRequestsAsync()' collect2: ld returned 1 exit status

Comment on attachment 250658 [details] [diff] [review] branch patch Approved for 1.8/1.8.0 branches, a=dveditz for drivers

Checked in on branches.

Verified fixed on Trunk, 1.8.1.2 and 1.8.0.10 tested with the steps to reproduce on comment#1 and also tested with the test cases Tested Builds: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.2pre) Gecko/20070207 BonEcho/2.0.0.2pre ID:2007020703 Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a2pre) Gecko/2007020611 Minefield/3.0a2pre Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.10pre) Gecko/20070207 Firefox/1.5.0.10pre ID:2007020706 also on Linux Fedora FC 6

This patch appears to have fixed Zalewski's onUnload vulnerability, bug 371321 -- you go, Jonas! patched before he even reported it and fix released a day after! On the down side, this appears to have broken every site using the backbase AJAX toolkit :-( bug 371576

The branch patch misspells Request in a method name, consistently of course: http://lxr.mozilla.org/mozilla1.8/search?string=Reqest /be