'Reflections' experiment doesn't load

RESOLVED FIXED

Status

()

Core
XSLT
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: sstangl, Assigned: hsivonen)

Tracking

unspecified
x86_64
All
Points:
---

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

(Whiteboard: chromeexperiments)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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

Updated

7 years ago
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 :(
(Assignee)

Comment 7

7 years ago
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...
(Assignee)

Comment 8

7 years ago
(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?
(Assignee)

Comment 9

7 years ago
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?
(Assignee)

Comment 10

7 years ago
Created attachment 486040 [details] [diff] [review]
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.
Attachment #486040 - Flags: review?(jonas)
blocking2.0: beta8+ → beta7+
(Assignee)

Comment 11

7 years ago
Created attachment 486296 [details] [diff] [review]
Special-case scripts from XSLT, now with all test files included in the patch
Attachment #486040 - Attachment is obsolete: true
Attachment #486296 - Flags: review?(jonas)
Attachment #486040 - Flags: review?(jonas)
(Assignee)

Comment 12

7 years ago
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.
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 13

7 years ago
(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: ??]
(Assignee)

Comment 15

7 years ago
(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.
(Assignee)

Updated

7 years ago
Whiteboard: chromeexperiments [ETA: needs answer to comment 12] → chromeexperiments [ETA: 1 Nov]
(Assignee)

Comment 17

7 years ago
Landed with the enumification patch:
http://hg.mozilla.org/mozilla-central/rev/8957830e22a8
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Whiteboard: chromeexperiments [ETA: 1 Nov] → chromeexperiments
You need to log in before you can comment on or make changes to this bug.