391177 - XSLT should reuse the inner window

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[John J. Barton]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7) Gecko/2007080210 GranParadiso/3.0a7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7) Gecko/2007080210 GranParadiso/3.0a7

In trying to support web pages generated by XSLT in Firebug I discovered that the window object associated with the .xml URL changed during the XSLT execution in ways other than transforming the document. Specifically I found that my eventListeners never fired and my window properties disappeared. 

Jonas Sicking says: "After some digging and help from jst we determined that what happens is that we once the xslt execution starts remove all registered eventhandlers and remove all set properties on the window. "

The direct impact is that Firebug can't know when the window is ready to be debugged.  In general all kinds of Firefox components and extensions watch windows by adding event handlers.

Reproducible: Always

Steps to Reproduce:
1. To build a complete focused test case I think one needs to build a Firefox extension that sets an eventListener on the xml window and a property then inspects them at the end.  As an alternative, Firebug branches/explore has a trace option that will expose the issue. Let me know if this is useful and I will send a XPI file and a trace from the current 3.0a7. Also Firebug should work on XSLT pages if this problem is fixed.
2.
3.
Actual Results:  
Firebug can't see the XSLT result because the window eventhandlers do not fire.

Expected Results:  
Firebug just works because the result from XSLT is a web page.

There is only a "General" category, the select box does not seem to have core components.  Obviously this should be under XSLT.

See also mozilla.dev.tech.xslt Re: XSLT processing model in Mozilla

news://news.mozilla.org:119/[e-mail]

Comment 1

[Boris Zbarsky [:bzbarsky]]

So is the issue just that we should be reusing the inner window when we make this SetDOMDocument call?  Is that in fact what we want to be doing?

If so, could we make use of the aClearScopeHint argument to SetNewWindow (which is currently unused) to indicate whether to force reusing the inner window?  e.g. force reusing if the arg is false.

Comment 2

[Johnny Stenback (:jst)]

If we do that we should rename the argument and make its name reflect what we're really doing here, or if we don't do that please file a bug (or remind me to file the bug :) to remove that argument...

Comment 3

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

Yeah, i think that's what we want to do.

Comment 4

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

Don't think this is blocking. Adding wanted as this affects firebug though.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Actually, one issue here is that scripts in the source document will run against its inner window.  Do we really want those scripts polluting the scope of the transformed document?  I would argue no.  Sicking, peterv, what do you think?

If we don't want said pollution, I think we should really give a better way for firebug and other extensions to hook into the point when we swap documents or something, and keep clearing the scope when we do the swap.

Another option is to disable the scriptloader once we know we're going to be doing an XSLT transform.  Not sure whether that's the behavior we want or not.

Comment 6

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

We should definitely disable the scriptloader. I thought we already did, but i can't find any such code right now.

Comment 7

[John J. Barton]

(In reply to comment #5)
> Actually, one issue here is that scripts in the source document will run
> against its inner window.  Do we really want those scripts polluting the scope
> of the transformed document?  I would argue no.  Sicking, peterv, what do you
> think?

I'm not sure what is the "source document" and "inner window" but debuggers 
want access to anything that affects the result.


 

Comment 8

[Boris Zbarsky [:bzbarsky]]

The source document is the XML document being transformed by the XSLT stylesheet.  There's an existing bug to make that available somehow.

The inner window is the Javascript execution scope attached to a document.

Not running scripts in the source document makes a lot of sense to me.  I'll look into that.

Comment 9

[John J. Barton]

My recollection from our meeting at the summit: Boris thought that Bug 342715 would provide Firebug with access to the input and output windows and we can sort out what to show the developer. We'll depend up on that one here.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Well... the original bug summary here is still correct.  We shouldn't really be wiping out the old window, since that can script up content, not just firebug.  It's pretty simple to write a testcase using window.open where things don't work right in the current setup for content code.

Fixing that doesn't depend on bug 342715.

Comment 11

[Rob Campbell [:rc] (:robcee)]

from MDC: "a new XML document is created based on the content of an existing document" kind of implies that a new inner window would get created for this, though I can see how that would cause scripting issues. Is this still a bug?

Comment 12

[John J. Barton]

You can investigate if any of the issues are fixed:
http://code.google.com/p/fbug/issues/list?can=2&q=Xslt
but I doubt there has been any change. However there is little push to fix this.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Rob, there are certainly cases where we create a new document without creating a new inner window.  See nsGlobalWindow::WouldReuseInnerWindow.

And one of the XLST module peers seems to think we should be doing so here (comment 3).

Comment 14

[Rob Campbell [:rc] (:robcee)]

Ok, this sounds like a fix is pretty do-able. Not sure how high a priority is though. Those bugs in the Firebug issue tracker are pretty old and we haven't heard anything on them for over a year. Moving this to P3.

Jonas, think this is feasible for 1.9.3?

Comment 15

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

I think so yeah. Just gotta remind me so that it doesn't fall between the cracks. If you want me to do it that is.

Comment 16

[Rob Campbell [:rc] (:robcee)]

well, we can evaluate after the 1.9.2 madness has ended. I guess the questions is, "is this a high enough priority?" and I'm not qualified to assess that. We can revisit later.

Comment 17

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

Attachment: patch to fix

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 473862 [details] [diff] [review]
patch to fix

r=jst if you add an assert (or even a runtime check) that the principal of the new document in nsGlobalWindow::SetNewDocument() is the same as the old document in the case where aForceReuseInnerWindow is true.

Comment 19

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

Checked in
http://hg.mozilla.org/mozilla-central/rev/3406ae8889f9