1182113 - Test loading XSLTs

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Comment 1

[Albert [:albert]]

I load an XSLT but I don't see that http://hg.mozilla.org/mozilla-central/file/0b901209064c/dom/xslt/xslt/txMozillaStylesheetCompiler.cpp#l467 is being executed.

    var processor = new XSLTProcessor();

    var xslt = document.implementation.createDocument("", "test_xslt", null);
    xslt.addEventListener("load", function() {
      processor.importStylesheet(xslt);
      var xml = document.implementation.createDocument("", "test_xml", null);
      xml.addEventListener("load", function() {

        var newDocument = processor.transformToDocument(xml);
        debug("newDocument: " + new XMLSerializer().serializeToString(newDocument));

      }, false);
      xml.load("test_xslt.xml");
    }, false);
    xslt.load("test.xslt");

Do I miss something?

Comment 2

[Josh Matthews [:jdm]]

From tracing backwards through the code, it looks like that the new channel is created when an XML document is being parsed that contains an XLS style link which uses XSLT.

Comment 3

[Josh Matthews [:jdm]]

I think importStylesheet is synchronous, so it ends up creating a txSyncCompileObserver (which uses nsSyncLoadService::LoadDocument) rather than txCompileObserver (which uses NS_NewChannel).

Comment 4

[(no longer active)]

Looks like Josh is on top of this.

Comment 5

[Albert [:albert]]

Attachment: Fix Request.mode

Fix Request.mode

Comment 6

[Albert [:albert]]

Attachment: Test

Tests added

Comment 7

[Albert [:albert]]

I added some tests that are working as expected, however when all tests finished I got an assertion:

[5484] ###!!! ASSERTION: Bad readystate: 'mDocument->IsXULDocument() || mDocument->GetReadyStateEnum() == nsIDocument::READYSTATE_INTERACTIVE || (mDocument->GetReadyStateEnum() == nsIDocument::READYSTATE_UNINITIALIZED && NS_IsAboutBlank(mDocument->GetDocumentURI()))', file /mozilla-central/layout/base/nsDocumentViewer.cpp, line 974
#01: nsDocumentViewer::LoadComplete(nsresult) (/mozilla-central/layout/base/nsDocumentViewer.cpp:966 (discriminator 1))
#02: nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) (/mozilla-central/docshell/base/nsDocShell.cpp:7342)
#03: nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) (/mozilla-central/docshell/base/nsDocShell.cpp:7160)
#04: nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) (/mozilla-central/uriloader/base/nsDocLoader.cpp:1250 (discriminator 1))
#05: nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) (/mozilla-central/uriloader/base/nsDocLoader.cpp:830 (discriminator 2))
#06: nsDocLoader::DocLoaderIsEmpty(bool) (/mozilla-central/uriloader/base/nsDocLoader.cpp:723)
#07: nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) (/mozilla-central/uriloader/base/nsDocLoader.cpp:608)
#08: nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) (/mozilla-central/netwerk/base/nsLoadGroup.cpp:643)
#09: nsDocument::DoUnblockOnload() (/mozilla-central/dom/base/nsDocument.cpp:9125 (discriminator 1))
#10: nsDocument::UnblockOnload(bool) (/mozilla-central/dom/base/nsDocument.cpp:9052)
#11: nsDocument::DispatchContentLoadedEvents() (/mozilla-central/dom/base/nsDocument.cpp:5111)
#12: void nsRunnableMethodArguments<>::apply<nsDocument, void (nsDocument::*)()>(nsDocument*, void (nsDocument::*)()) (/mozilla-central/obj-firefox/dom/base/../../dist/include/nsThreadUtils.h:662 (discriminator 3))
#13: nsRunnableMethodImpl<void (nsDocument::*)(), true>::Run() (/mozilla-central/obj-firefox/dom/base/../../dist/include/nsThreadUtils.h:870)
#14: nsThread::ProcessNextEvent(bool, bool*) (/mozilla-central/xpcom/threads/nsThread.cpp:874 (discriminator 1))
#15: NS_ProcessNextEvent(nsIThread*, bool) (/mozilla-central/xpcom/glue/nsThreadUtils.cpp:277)
#16: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/mozilla-central/ipc/glue/MessagePump.cpp:95)
#17: MessageLoop::RunInternal() (/mozilla-central/ipc/chromium/src/base/message_loop.cc:235)
#18: MessageLoop::RunHandler() (/mozilla-central/ipc/chromium/src/base/message_loop.cc:228)
#19: MessageLoop::Run() (/mozilla-central/ipc/chromium/src/base/message_loop.cc:201)
#20: nsBaseAppShell::Run() (/mozilla-central/widget/nsBaseAppShell.cpp:158)
#21: nsAppStartup::Run() (/mozilla-central/toolkit/components/startup/nsAppStartup.cpp:281)
#22: XREMain::XRE_mainRun() (/mozilla-central/toolkit/xre/nsAppRunner.cpp:4292)
#23: XREMain::XRE_main(int, char**, nsXREAppData const*) (/mozilla-central/toolkit/xre/nsAppRunner.cpp:4389)
#24: XRE_main (/mozilla-central/toolkit/xre/nsAppRunner.cpp:4478)
#25: do_main (/mozilla-central/browser/app/nsBrowserApp.cpp:212)
#26: main (/mozilla-central/browser/app/nsBrowserApp.cpp:399)
#27: __libc_start_main (/build/buildd/eglibc-2.19/csu/libc-start.c:321)
#28: _start (/mozilla-central/obj-firefox/dist/bin/firefox)
#29: ??? (???:???)

Is it related with the loading iframe?

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8656547 [details] [diff] [review]
Fix Request.mode

Review of attachment 8656547 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/xslt/xslt/txMozillaStylesheetCompiler.cpp
@@ +513,5 @@
> +    // based on nsILoadInfo securityFlags instead. This block will be removed
> +    // when Request.mode set correctly.
> +    nsCOMPtr<nsIHttpChannelInternal> httpChannelInt = do_QueryInterface(channel);
> +    if (httpChannelInt) {
> +      httpChannelInt->SetCorsMode(nsIHttpChannelInternal::CORS_MODE_SAME_ORIGIN);

Can you point me at the spec or something else that indicates this needs to be same origin?  I can't find the reference?  Currently it seems the code sets an nsCorsListenerProxy which suggests it should use CORS, not same-origin.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8656548 [details] [diff] [review]
Test

Review of attachment 8656548 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/test/serviceworkers/xslt_worker.js
@@ +15,5 @@
> +onfetch = function(event) {
> +  if (event.request.url.includes('test.xsl')) {
> +    if (testType == 'synthetic') {
> +      if (event.request.mode != 'same-origin') {
> +        event.respondWith(Promise.reject());

Instead of Promise.reject(), can you please use Response.error()?  I think its a little clearer.

@@ +42,5 @@
> +      var url = "http://example.com/tests/dom/workers/test/serviceworkers/xslt/xslt.sjs?" + escape(xslt);
> +      event.respondWith(fetch(url, { mode: 'no-cors' }));
> +    }
> +    else {
> +      event.respondWith(Promise.reject()); 

nit: trailing whitespace

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #8)
> Comment on attachment 8656547 [details] [diff] [review]
> Fix Request.mode
> 
> Review of attachment 8656547 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/xslt/xslt/txMozillaStylesheetCompiler.cpp
> @@ +513,5 @@
> > +    // based on nsILoadInfo securityFlags instead. This block will be removed
> > +    // when Request.mode set correctly.
> > +    nsCOMPtr<nsIHttpChannelInternal> httpChannelInt = do_QueryInterface(channel);
> > +    if (httpChannelInt) {
> > +      httpChannelInt->SetCorsMode(nsIHttpChannelInternal::CORS_MODE_SAME_ORIGIN);
> 
> Can you point me at the spec or something else that indicates this needs to
> be same origin?  I can't find the reference?  Currently it seems the code
> sets an nsCorsListenerProxy which suggests it should use CORS, not
> same-origin.

I also asked about this in #content.  It seems maybe xslt used to be same-origin before, but is now CORS:

  http://logs.glob.uno/?c=content#c319905

Comment 11

[Josh Matthews [:jdm]]

It is, but I don't know what the problem is without stepping through it in gdb. Perhaps See what happens if you remove the iframe each time and make a new one with the new src value?

Comment 12

[Albert [:albert]]

Attachment: Test

Changes from comment 9

Comment 13

[Albert [:albert]]

(In reply to Josh Matthews [:jdm] from comment #11)
> It is, but I don't know what the problem is without stepping through it in
> gdb. Perhaps See what happens if you remove the iframe each time and make a
> new one with the new src value?

It seems it is not a problem with the iframe, what you proposed fails as well. I changed the iframe with a window open and it also fails. I've seen that same code removing the service worker interception it does not fail.

At https://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#966 I see that document readystate is nsIDocument::READYSTATE_INTERACTIVE when there is no service worker interception and nsIDocument::READYSTATE_UNINITIALIZED when the service worker is working.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: bug-1182113.patch

I spent a bit of time debugging this issue and it's not related to SW. The same assertion is triggered if a simple test.xml+test.xsl is executed directly. Also in a new clean profile.

The problem happens because of <xsl:output method="text"/>

This doesn't mean we don't have to fix the issue, but just that it's not a blocker for SW.
I'll file a separate bug for fixing the crash.

This new patch removes just that line. I guess you can push it to m-i.

Comment 15

[Albert [:albert]]

Attachment: Test

Rebase

Comment 16

[Albert [:albert]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=445836673282

Comment 17

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec9dc8f952c5

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ec9dc8f952c5