604660 - 'Reflections' experiment doesn't load

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Sean Stangl [:sstangl]]

The 'Chrome Experiment' located at
http://www.chromeexperiments.com/detail/reflections/
no longer loads with the latest nightly, apparently as of October 13.

The Error Console yields '$ is not defined'.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

The script in question here is loaded via an XSLT document. Need to debug more, but that seems like a decent starting place.

Comment 2

[Peter Van der Beken [:peterv]]

The XSLT inserts a bunch of scripts, and it looks like they don't execute in the order that they're inserted. The scripts rely on that though, and if this wasn't using XSLT we'd block the parser after each script load so they would execute in order. Jonas says this is a regression from a change Henri made, so assigning to him.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Note that in other UAs the outout of XSLT is reparsed, so _does_ go through the parser codepath.

Comment 4

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

Indeed. But since we do XSLT differently we might need to add special code here.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Right, exactly.  My point was that in XSLT output we do need to preserve script insertion order when executing, since de-facto all other UAs do.

Comment 6

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

Oh, I just realized, in this case we even need to preserve order between inline and external scripts :(

Comment 7

[Henri Sivonen (:hsivonen)]

It seems to me the fix that would most closely match other browsers would be:
 1) Marking XSLT-created scripts NS_FROM_PARSER_NETWORK.
 2) Blocking the XSLT output module on NS_ERROR_HTMLPARSER_BLOCK.

I wonder if point #2 is feasible...

Comment 8

[Henri Sivonen (:hsivonen)]

(In reply to comment #7)
> It seems to me the fix that would most closely match other browsers would be:
>  1) Marking XSLT-created scripts NS_FROM_PARSER_NETWORK.
>  2) Blocking the XSLT output module on NS_ERROR_HTMLPARSER_BLOCK.
> 
> I wonder if point #2 is feasible...

peterv tells me point #2 isn't feasible. :-(

Sicking, how about this:
 1) I land bug 594339.
 2) I create a new value for the enum: FROM_PARSER_XSLT
 3) I write a patch that flips around the default for HTMLScriptElement::async for script-created scripts. (To avoid side effects from landing bug 602838 alone.)
 4) I land the patch for bug 602838 and the patch from the above step together.
 5) I make txMozillaXMLOutput pass FROM_PARSER_XSLT or FROM_PARSER_FRAGMENT to the new element creator depending on the state of mCreatingNewDocument. I add cases to nsScriptLoader so that FROM_PARSER_XSLT scripts go to the new array introduced by the patch patch for bug 602838 if the script is non-async, non-defer and external or if it's inline and the array is not empty (and return NS_ERROR_HTMLPARSER_BLOCK). I make FROM_PARSER_XSLT with the defer attribute go to the defer list. I call PreventExecution on scripts in the FROM_PARSER_FRAGMENT case.
 6) I make txMozillaXMLOutput call DoneAddingChildren or DoneCreatingElement on all elements that require either of those calls when they aren't script-created.

I believe the above steps would fix this bug in as good a manner as this bug can be fixed given the lack of suspendability of the transformer.

Sicking, OK to proceed according to the above plan?

Comment 9

[Henri Sivonen (:hsivonen)]

Actually, thinking about this more, maybe XSLT scripts should have their own array in nsScriptLoader to avoid introducing unwanted ordering guarantees when XSLT-inserted scripts insert non-async script-inserted scripts.

So let's scratch steps 3 and 4 above and replace them with a new array for XSLT-inserted scripts.

Sicking, if I proceed according to *this* plan, can I safely assume that the fix for bug 594339 and the fix for this bug will land before the fix for bug 602838 if we opt to land that one at all?

Comment 10

[Henri Sivonen (:hsivonen)]

Attachment: Special-case scripts from XSLT

(In reply to comment #9)
> Sicking, if I proceed according to *this* plan, can I safely assume that the
> fix for bug 594339 and the fix for this bug will land before the fix for bug
> 602838 if we opt to land that one at all?

I proceeded according to this plan.

Comment 11

[Henri Sivonen (:hsivonen)]

Attachment: Special-case scripts from XSLT, now with all test files included in the patch

Comment 12

[Henri Sivonen (:hsivonen)]

Sicking, considering that this is now a beta7 blocker, when reviewing, could you please indicate if it's OK to take the fix for bug 594339 along as a dependency for this patch or whether I need to change this patch not build on the fix for bug 594339.

Comment 13

[Henri Sivonen (:hsivonen)]

(In reply to comment #8)
> (In reply to comment #7)
> > It seems to me the fix that would most closely match other browsers would be:
> >  1) Marking XSLT-created scripts NS_FROM_PARSER_NETWORK.
> >  2) Blocking the XSLT output module on NS_ERROR_HTMLPARSER_BLOCK.
> > 
> > I wonder if point #2 is feasible...
> 
> peterv tells me point #2 isn't feasible. :-(

FWIW, the infeasibility of point #2 is bug 426366.

Comment 14

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

Comment on attachment 486296 [details] [diff] [review]
Special-case scripts from XSLT, now with all test files included in the patch

>@@ -572,50 +576,68 @@ nsScriptLoader::ProcessScriptElement(nsI
...
>+    if (aElement->GetParserCreated() == FROM_PARSER_XSLT) {
>+      // Need to maintain order for XSLT-inserted scripts
>+      NS_ASSERTION(!mParserBlockingRequest,
>+          "Parser-blocking scripts and XSLT scripts on the same doc!");

"in the same doc"

Once more further down.

>@@ -851,44 +883,52 @@ nsScriptLoader::ProcessPendingRequests()
>   if (mParserBlockingRequest &&
>       !mParserBlockingRequest->mLoading &&
>       ReadyToExecuteScripts()) {
>     request.swap(mParserBlockingRequest);
>     // nsContentSink::ScriptAvailable unblocks the parser
>     ProcessRequest(request);
>   }
> 
>+  if (ReadyToExecuteScripts()) {
>+    while (!mXSLTRequests.IsEmpty() && !mXSLTRequests[0]->mLoading) {
>+      request.swap(mXSLTRequests[0]);
>+      mXSLTRequests.RemoveElementAt(0);
>+      ProcessRequest(request);
>+    }
>+  }

Doesn't this need to be

while (ReadyToExecuteScripts() &&
       !mXSLTRequests.IsEmpty() &&
       !mXSLTRequests[0]->mLoading) {
  ...
}

? If nothing else it seems safer to do it that way.

r=me with that fixed or explained.

Comment 15

[Henri Sivonen (:hsivonen)]

(In reply to comment #12)
> Sicking, considering that this is now a beta7 blocker, when reviewing, could
> you please indicate if it's OK to take the fix for bug 594339 along as a
> dependency for this patch or whether I need to change this patch not build on
> the fix for bug 594339.

Any response to this?

Comment 16

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

Assuming that code *only* contains enumification then I'd say it's ok to take that too for b7. If so a=me for landing on m-c asap.

Comment 17

[Henri Sivonen (:hsivonen)]

Landed with the enumification patch:
http://hg.mozilla.org/mozilla-central/rev/8957830e22a8