Closed Bug 604660 Opened 10 years ago Closed 10 years ago

'Reflections' experiment doesn't load

Categories

(Core :: XSLT, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: sstangl, Assigned: hsivonen)

Details

(Whiteboard: chromeexperiments)

Attachments

(1 file, 1 obsolete file)

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'.
blocking2.0: --- → ?
Whiteboard: chromeexperiments
The script in question here is loaded via an XSLT document. Need to debug more, but that seems like a decent starting place.
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.
Assignee: general → hsivonen
Component: JavaScript Engine → XSLT
QA Contact: general → xslt
blocking2.0: ? → beta8+
Note that in other UAs the outout of XSLT is reparsed, so _does_ go through the parser codepath.
Indeed. But since we do XSLT differently we might need to add special code here.
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.
Oh, I just realized, in this case we even need to preserve order between inline and external scripts :(
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...
(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?
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?
Attached patch Special-case scripts from XSLT (obsolete) — Splinter Review
(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.
Attachment #486040 - Flags: review?(jonas)
blocking2.0: beta8+ → beta7+
Attachment #486040 - Attachment is obsolete: true
Attachment #486296 - Flags: review?(jonas)
Attachment #486040 - Flags: review?(jonas)
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.
Status: NEW → ASSIGNED
(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 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.
Attachment #486296 - Flags: review?(jonas) → review+
Whiteboard: chromeexperiments → chromeexperiments [ETA: ??]
(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?
Whiteboard: chromeexperiments [ETA: ??] → chromeexperiments [ETA: needs answer to comment 12]
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.
Whiteboard: chromeexperiments [ETA: needs answer to comment 12] → chromeexperiments [ETA: 1 Nov]
Landed with the enumification patch:
http://hg.mozilla.org/mozilla-central/rev/8957830e22a8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: chromeexperiments [ETA: 1 Nov] → chromeexperiments
You need to log in before you can comment on or make changes to this bug.