214081 - [FIX]Content sink can get confused about pending script loads

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

The basic problem is that the content sink has some logic to guess whether a
script will require the parser to be blocked.  Instead of guessing, it should
use something reliable like a return value or something....

In fact, the sane way to do this may be to move the parser blocking and
unblocking into the script element itself (sort of the way the stylesheet
linking stuff works).

I'll be attaching a minimal testcase reduced from that website

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: JS for testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

This should actually finish loading and show some text or something.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Or we could take the hacks out and actually fire ScriptAvailable like we're supposed to

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 128643 [details] [diff] [review]
Or we could take the hacks out and actually fire ScriptAvailable like we're supposed to

- In nsHTMLScriptElement::MaybeProcessScript():

-  if (mIsEvaluated || mEvaluating || !mDocument || !mParent ||
-      !mDocument->IsScriptEnabled()) {
+  if (mIsEvaluated || mEvaluating || !mDocument || !mParent) {
     return;
   }

Don't we still want the !mDocument->IsScriptEnabled() check here? W/o that
check, a script element that's added to a document where scripts are disabled
(from a window where scripts are enabled) could still cause the script to load
(though not execute), right?

Comment 5

[Boris Zbarsky [:bzbarsky]]

No.  Notice that the ScriptLoader does this check with this patch and if scripts
are disabled just bails (but notifies ScriptAvailable with an error, unblocking
the parser, which is key -- otherwise disabling scripts means you can't load any
pages with a <script> tag in them).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Guys, any chance of reviews sometime today?  Like in the next 6-7 hours?

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 128643 [details] [diff] [review]
Or we could take the hacks out and actually fire ScriptAvailable like we're supposed to

r+sr=jst

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

This caused regression bug 214874 -- scriptloader needs to check not only
whether scripts are enabled on the document, but also whether they are enabled
on the current scriptcontext.