Closed Bug 83557 Opened 23 years ago Closed 23 years ago

XMLSerializer and DOMParser do not work in embedded case

Categories

(Core :: XML, defect, P1)

Product:
Core
Component:
XML
Platform:
x86
Windows NT
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.2

People

(Reporter: hjtoi-bugzilla, Assigned: hjtoi-bugzilla)

References

Details

(Whiteboard: [fixed on trunk, need for embedding])

Attachments

(5 files)

HTML testcase that works
325 bytes, text/html
Details
XUL testcase that works
623 bytes, application/vnd.mozilla.xul+xml
Details
XUL file that does not work, needs JS file too (save these to disc)
500 bytes, text/plain
Details
JS file that does not work, needs XUL file too (save these to disc)
1.25 KB, text/plain
Details
Proposed fix for XMLHttpRequest
23.27 KB, patch
Details | Diff | Splinter Review 
Using winEmbed or viewer we do not get into the C++ implementations of these
classes. In fact, the script execution seems to stop at the location where you
try to call their member functions, but you can't even catch the error with
try-catch pair in JS. They work fine in browser.

My first guess is the xpcdom work, although I have no idea why things would work
in browser and not in viewer.

Important internal customer needs this for 0.9.1
Target Milestone: --- → mozilla0.9.1
Whiteboard: investigating
This is strange... The first two testcase work fine with mozilla the browser and
winEmbed.

The 3rd and 4th attachments form the testcase that works in mozilla the browser,
but does not work in winEmbed or viewer. XMLHttpRequest seems to still work
fine. The C++ constructors for nsDOMParser and nsDOMSerializer are called, but
even though I have breakpoints in nsDOMParser::ParseFromString() and
nsDOMSerializer::SerializeToString() I never break in them.

Tom, could you please test that your applications still work in some of the
nightly builds (any build from last couple of weeks would be fine)? You can just
state the end results here(yes/no), and if there is some sensitive information
you can send that to me in email.
jst: Could you take a look at what is going on here? I don't think I know how to
track this one down...
This bug appears also in nightly build 05-10-04. Is there a pre-xpcdom build
anywhere to test?
I had a look at this, and there's 4 problems here.

1) mfcembed builds don't have a way of reporting JS errors (that I know) so any
exception that's thrown get lost and execution of JS just silently stops.

2) In the debugger, I see "uncaught exception: enablePrivilege not granted" when
loading the last testcase, this happens since the security manager tries to ask
the user if the user alows enabling this privilege by calling into
CPromptService::ConfirmEx(), which simply returns NS_ERROR_NOT_IMPLEMENTED.
Because of this we end up throwing a JS exception (which isn't shown to the user
anywhere).

3) The event handlers (onload or onerror) fired by the XMLHttpRequest object
execute on the "safe context", this used to be the hidden window, and still is
the hidden window in mozilla, but in the embedding case there is no hidden
window so we end up executing the event handlers on one of XPConnects internal
safe contexts. The XPConnect internal safe context's global object doesn't have
nsISupports private data that implements nsIScriptObjectPrincipal so we're
unable to get the object principal and this results in the security check
failing when trying to call par.parseFromString(...).

4) The security manager doesn't throw an exception in the above failure case so
even if there was an error reporting mechanism in the embedding app, the user
wouldn't see why the script stopped executing.

It seems unlikely that this would be related to the XPCDOM landing, it probably
broke when danm removed the hidden window from the embedding builds.

Embedding people need to fix 1) and 2), cc:ing valeski and adam lock.

You, Heikki, needs to fix the XMLHttpRequest object to push the JS context where
the XMLHttpRequest was created onto the context stack when calling the onload
and onerror handlers to get them to execute on the same context. And while
you're at it, replace the nsXMLHttpRequestScriptListener code with simple calls
into XPConnect wrapped JS function objects, just like the DOM event code does
(i.e. change the type of nsIXMLHttpRequest::onload and ::onerror to
nsIDOMEventListener and you could probably remove most of the JS specific code
from nsXMLHttpRequest).

Mitchell needs to make the security manager throw an exception if it can't get
the subject principal from an object in a case like this.

jst said the fix in bug 83332 could be used as a sample to fix part of this.
Status: NEW → ASSIGNED
The patch I just attached makes XMLHttpRequest use the new XPConnect 'features'
which lets us take out a bunch of JS specific code. The change also makes
XMLHttpRequest be a generic DOM class from the security managers point of view
which lets us remove nsISecurityCheckedComponent from nsXMLHttpRequest, AFAIK
this is OK, but someone needs to verify that part.
The fix looks good to me, although there are aspects of it that I do not yet
fully understand. I have run this through all the testcases I have about
XMLHttpRequest, and it is working perfectly in mozilla and winEmbed. mfcEmbed
works as long as it is not being asked for something that has not been
implemented in it (in which case some methods just fail - I will still check if
the failure code could be improved). I will also make some new tests to check
the security, although it felt like it worked as designed with the tests I have.
Anyway, I feel confident enough to give r=heikki.

Mitch, could you also take a look? Maybe there is some additional code that
could be removed/changed now that XMLHttpRequest is more like the rest of the
DOM objects (i.e. remove some XMLHttpRequest specific security code)?
Whiteboard: investigating → [fixinhand by jst] r=heikki
Blocks: 83989
We'd love to have this by 11:59pm Tuesday June 5!
We should confirm that the reference to a script context from the request object
isn't circular. If it doesn't, then sr=vidur.

I'm not particularly fond of the dependency that's introduced from XMLExtras to
the DOM module to get class info. It's not a requirement for now, but I'd like
to eventually see the component produce its own class info.
I am moving this off to 0.9.2 (for the trunk).

However, if and when there is going to be an embedding release off of 0.9.1 this
needs to get in on the branch, or we need to create a new embedding branch from
0.9.1 for this one.
Priority: -- → P1
Target Milestone: mozilla0.9.1 → mozilla0.9.2
a= asa@mozilla.org for checkin to the trunk.
(on behalf of drivers)
To followup on Vidur's questions about holding a strong ref to the
nsISriptContext...

Holding a strong reference to the script context in an XMLHttpRequest *will*
create a circular reference that will keep the context (obviously) and the
global object in the context (including everything that's reachable from the
global object) around for the lifetime of the XMLHttpRequest object, which can
be bound to the lifetime of the global object if the XMLHttpRequest object
itself is reachable from the global object. This is, however, ok. Circular
refences like these should *not* cause leaks, circular references should be
properly cleaned up by the JS GC, it might take a few GC runs, but they should
be cleaned up. If this is not the case then there's a problem (or I overlooked
something).
I checked in the fix on the trunk. I am leaving this bug still open so that it
stays on the radar for embedding needs. The status bar reflects the current
situation.
Whiteboard: [fixinhand by jst] r=heikki → [fixed on trunk, need for embedding]
I am marking this fixed as I believe embedding folks got their bits from 0.9.2
which contains this fix (even the branch). This is not in 0.9.1 branch, though,
in case someone is wondering.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Marking verified per last comments.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: