364692 - [Fx 2.0.0.1/1.5.0.9 regression] Can't view talkbacks on ynet.co.il

Status: VERIFIED FIXED

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

Description

[Uri Bernstein]

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.

Comment 1

[Uri Bernstein]

Attachment: testcase

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

Comment 2

[Uri Bernstein]

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

Comment 3

[Uri Bernstein]

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

Comment 4

[Uri Bernstein]

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.

Comment 5

[Jay Patel [:jay]]

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.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: branch patch

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.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: better testcase

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.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Trunk patch

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.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Landed the trunk patch.

Comment 14

[(not currently active) Ted Mielczarek]

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 15

[Daniel Veditz [:dveditz]]

Comment on attachment 250658 [details] [diff] [review]
branch patch

Approved for 1.8/1.8.0 branches, a=dveditz for drivers

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in on branches.

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Daniel Veditz [:dveditz]]

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

Comment 19

[Brendan Eich [:brendan]]

The branch patch misspells Request in a method name, consistently of course:

http://lxr.mozilla.org/mozilla1.8/search?string=Reqest

/be