306678 - instance elements does not work in documents without windows

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[alexander :surkov (:asurkov)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20050821 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20050821 Firefox/1.6a1

I load xml document dynamicaly witch contains xforms model. When I call
model.getInstanceDocument(null) then I get an exception.

Reproducible: Always

Comment 1

[alexander :surkov (:asurkov)]

Attachment: xforms xml

Comment 2

[alexander :surkov (:asurkov)]

Attachment: testcase

Comment 3

[alexander :surkov (:asurkov)]

I tested on SeaMonkey 1.0a (Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
rv:1.8b3) Gecko/20050830 SeaMonkey/1.0a)

Comment 4

[Allan Beaufour]

(In reply to comment #2)
> Created an attachment (id=194522) [edit]
> testcase

The testcase never calls onload() does it?

It took me a while to figure out what you were trying to do here... If I
understand it correctly you try to get the XForms instance document from a model
in dynamically loaded and parsed xml document. So what actually fails is:
var doc = domparser.parseFromString(rep, "text/xml");
var model = doc.getElementById('protocol');
var idoc = model.getInstanceDocument(null);

As far as I can see the parser never triggers XTF, so our XForms code is never
reached.

Comment 5

[Allan Beaufour]

(In reply to comment #4)
> var model = doc.getElementById('protocol');

Actually, this fails for some reason...

Comment 6

[Allan Beaufour]

Attachment: XHTML testcase

Here's a somewhat reduced testcase in XHTML instead.

It has two bugs:
1) doc.getElementById does not get the model element
2) XTF is never invoked on the parsed document

Comment 7

[Allan Beaufour]

(In reply to comment #6)
> 1) doc.getElementById does not get the model element

Doron answered this one. You need to define what the id attribute is called for
a plain XML document. XHTML defines it as "id", but this document is just plain XML.

Comment 8

[Allan Beaufour]

(In reply to comment #6)
> 2) XTF is never invoked on the parsed document

Hmmm, my xforms build was a bit horked. I now get the exception. I suspect that
it might be related to bug 294612.

Comment 9

[Allan Beaufour]

(In reply to comment #8)
> (In reply to comment #6)
> > 2) XTF is never invoked on the parsed document
> 
> Hmmm, my xforms build was a bit horked. I now get the exception. I suspect that
> it might be related to bug 294612.

It is not. It's actually a "feature":
http://lxr.mozilla.org/seamonkey/source/extensions/xforms/nsXFormsInstanceElement.cpp#486
because of bug 267990.

Comment 10

[Allan Beaufour]

smaug and I talked about it on IRC, and he suggested that we set a
"IsInstanceDocument" property on the _instance document_, and then do not load
external instances in documents with that property.

That means that we will not let instance documents load external instances,
which seems reasonable. Having a "live" instance document seems weird in the
first place, but well, somebody might find a reason for having it... At least
the "do not work in documents without windows"-restriction is too restrictive,
and this will fix that at least.

Comment 11

[Allan Beaufour]

Attachment: Patch

This patch sets and checks a property on the document, before loading external
instances -- smaug's recipe.

Fixes Alexander's testcase, and also passes the one from bug 267990.

[ Made with very little sleep -- no guarantees to quality -- caution should be
taken :-) ]

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #7)
> (In reply to comment #6)
> > 1) doc.getElementById does not get the model element
> 
> Doron answered this one. You need to define what the id attribute is called for
> a plain XML document. XHTML defines it as "id", but this document is just
plain XML.

Er, not document, but element. And for XTF elements ID attribute is always called
'id'

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 196167 [details] [diff] [review]
Patch

>+  // Check whether we are an instance document ourselves
>+  nsCOMPtr<nsIDOMDocument> domDoc;
>+  mElement->GetOwnerDocument(getter_AddRefs(domDoc));
>+  nsCOMPtr<nsIDocument> doc(do_QueryInterface(domDoc));
>+  if (doc) {
>+    nsresult tmp;
>+    doc->GetProperty(nsXFormsAtoms::isInstanceDocument, &tmp);
>+    if (NS_SUCCEEDED(tmp)) {

Couldn't you do just
if (doc->GetProperty(nsXFormsAtoms::isInstanceDocument)) {


With that r=me

Comment 14

[Doron Rosenberg (IBM)]

Comment on attachment 196167 [details] [diff] [review]
Patch

>Index: resources/locale/en-US/xforms.properties
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/locale/en-US/xforms.properties,v
>retrieving revision 1.11
>diff -u -p -U8 -r1.11 xforms.properties
>--- resources/locale/en-US/xforms.properties	23 Aug 2005 21:20:28 -0000	1.11
>+++ resources/locale/en-US/xforms.properties	15 Sep 2005 14:18:31 -0000
>@@ -52,13 +52,14 @@ instanceParseError   = XForms Error (13)
> submitSendOrigin     = XForms Error (14): Security check failed! Trying to submit data to a different domain than document
> instanceLoadOrigin   = XForms Error (15): Security check failed! Trying to load instance data from a different domain than document
> controlBindError     = XForms Error (16): Could not bind control to instance data
> labelLinkLoadOrigin  = XForms Error (17): Security check failed! Trying to load label data from a different domain than document
> labelLink1Error      = XForms Error (18): External file (%S) for Label element not found
> labelLink2Error      = XForms Error (19): Failed to load Label element from external file: %S
> invalidSeparator     = XForms Error (20): Submission separator may only be either "&" or ";", but found "%S".
> instanceBindError    = XForms Error (21): Submission failed trying to replace instance document '%S'.  Instance document doesn't exist in same model as submission element.
>+instanceInstanceLoad = XForms Error (22): Instance document trying to load external instance: %S
> 

Shouldn't that be "Instance document failed trying..." ? (missing failed)

Comment 15

[alexander :surkov (:asurkov)]

Allan, please comment your patch. I don't understand a little. Is it mean that
when I load xforms model from file then I can get instance document by
model.getInstanceDocument() method? If is true then what means "That means that
we will not let instance documents load external instances"?

Comment 16

[Allan Beaufour]

(In reply to comment #15)
> Allan, please comment your patch. I don't understand a little. Is it mean that
> when I load xforms model from file then I can get instance document by
> model.getInstanceDocument() method?

Yes.

> If is true then what means "That means that we will not let instance documents
> load external instances"?

That means that you cannot load an instance document which itself loads external
instances too. In other words, you can not have
<model>
 <instance src="bar.xml"/>
</model>

with a bar.xml:
<model>
  <instance src="foo.xml/>  <-- this will fail
</model>

but bar.xml:
<model>
  <instance>
    <data></data>
  </instance>
</model>
will work.

Comment 17

[Allan Beaufour]

Checked in to trunk

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> That means that you cannot load an instance document which itself loads external
> instances too. 

It looks strange. What goal was pursued when such restriction was introduced?

Comment 19

[Allan Beaufour]

(In reply to comment #18)
> (In reply to comment #16)
> > That means that you cannot load an instance document which itself loads external
> > instances too. 
> 
> It looks strange. What goal was pursued when such restriction was introduced?

To avoid loops:

foo.xml:
<model>
  <instance src="foo.xml"/>
</model>

Comment 20

[Boris Zbarsky [:bzbarsky]]

So... Is loading instance documents for a document that's in bfcache ok?

Comment 21

[Allan Beaufour]

(In reply to comment #20)
> So... Is loading instance documents for a document that's in bfcache ok?

I do not really know to much about how bfcache works :| The only change is that
we do not bail when the GetScriptGlobalObject is null, but when the main
document is an instance document.

Comment 22

[Boris Zbarsky [:bzbarsky]]

A document in bfcache is stored in memory and gives out null for the script
global object.  It's not currently being rendered, has no container, but _does_
have a presentation.  Which also has no container.

It can also suddenly start being rendered.

So what are the implications of loading an instance document?  What things will
it trigger?  We're still sorting out the security implications of any sort of
change happening on a document that's in bfcache, so it would be good to know
what sort of things we need to look for.

Comment 23

[Allan Beaufour]

(In reply to comment #22)
> A document in bfcache is stored in memory and gives out null for the script
> global object.  It's not currently being rendered, has no container, but _does_
> have a presentation.  Which also has no container.
> 
> It can also suddenly start being rendered.

Ok so basically a loaded document "just" has it's container removed/reattached,
when being put to and taken from the bfcache, if I understand you right.
 
> So what are the implications of loading an instance document?  What things will
> it trigger?  We're still sorting out the security implications of any sort of
> change happening on a document that's in bfcache, so it would be good to know
> what sort of things we need to look for.

Everything in _pure_ XForms is triggered by user interaction for now, but of
course somebody could a script timer and trigger loading. I guess timers are
disabled, or?

I guess this patch means that our instance loading is not influenced by whether
the document exists in a window or in the bfcache. So if something needs to be
disabled when we are in the bfcache, we need to add that explicitly.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> "just" has it's container removed/reattached

Container and script global.  And yes, JS timers (via window.setTimeout and
window.setInterval) are disabled while in bfcache.  If there is C++ code in
XForms that sets XPCOM timers, those are different, of course.

As for whether something needs to be disabled when in bfcache, I don't know
enough about XForms and the implications loading instances has to tell...

Can loading the instance cause script in the affected document to run?

Comment 25

[Allan Beaufour]

(In reply to comment #24)
> > "just" has it's container removed/reattached
> 
> Container and script global.  And yes, JS timers (via window.setTimeout and
> window.setInterval) are disabled while in bfcache.  If there is C++ code in
> XForms that sets XPCOM timers, those are different, of course.

I'm pretty sure we're not using timers.

> As for whether something needs to be disabled when in bfcache, I don't know
> enough about XForms and the implications loading instances has to tell...

Well, in general everything is user triggered somehow. We do not engange in
anything "on our own" except on page load.

> Can loading the instance cause script in the affected document to run?

Yes, events will be fired if controls are bound to data in the changed instance
document.

If timers are disabled, I can only see a problem if a document is moved to
bfcache _while_ it is loading an instance. Then it will fire events, that can
trigger scripts, etc. But I guess that goes for all actions... if the document
is moved while processing is happening.

Comment 26

[Boris Zbarsky [:bzbarsky]]

If there is a scriptable way to load an instance, that script could run (eg in a
different frame) after the document is in bfcache.

Comment 27

[Allan Beaufour]

(In reply to comment #26)
> If there is a scriptable way to load an instance, that script could run (eg in a
> different frame) after the document is in bfcache.

Is that a question, a fact, or? From a user perspective I would not expect
anything to happen on a page that I have navigated away from, so that's bad I
guess...

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment 26 is a fact, not a question.

Comment 29

[aaronr]

checked into branch 20051004