920015 - Expose DOM URL to js modules

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Could we access global "URL" from js modules? Pretty please?

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[(no longer active)]

Comment on attachment 809213 [details] [diff] [review]
patch

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

r=me with the test fix.

::: dom/base/test/test_url.xul
@@ +16,5 @@
> +  const Cu = Components.utils;
> +
> +  // Import our test JSM. We first strip the filename off
> +  // the chrome url, then append the jsm filename.
> +  var base = /.*\//.exec(window.location.href)[0];

Fancy stuff!

::: dom/workers/test/test_url.xul
@@ +20,5 @@
> +    {
> +      waitForWorkerFinish();
> +
> +      Components.utils.import("chrome://mochitests/content/chrome/dom/workers/test/file_url.jsm");
> +      checkFromJSM(ok, is, finish);

Am I missing something?  You're not running this test in a worker.

Comment 3

[Masatoshi Kimura [:emk]]

Comment on attachment 809213 [details] [diff] [review]
patch

>+++ b/dom/base/URL.cpp
>@@ -37,20 +37,17 @@ URL::WrapObject(JSContext* aCx, JS::Hand
>-  if (!window) {
>-    aRv.Throw(NS_ERROR_UNEXPECTED);
>-    return nullptr;
>-  }
>+  // Note: we can be window-less.

I think GlobalObject should be passed to the constructor and the type of mWindow should be changed to nsCOMPtr<nsISupports>.

>@@ -66,20 +63,17 @@ URL::Constructor(const GlobalObject& aGl
>-  if (!window) {
>-    aRv.Throw(NS_ERROR_UNEXPECTED);
>-    return nullptr;
>-  }
>+  // Note: we can be window-less.

Same here.

>@@ -138,64 +132,79 @@ URL::CreateObjectURL(const GlobalObject&
>-  NS_PRECONDITION(!window || window->IsInnerWindow(),
>-                  "Should be inner window");
>+  if (window) {
>+    NS_PRECONDITION(!window || window->IsInnerWindow(),
>+                    "Should be inner window");
> 
>-  if (!window || !window->GetExtantDoc()) {
>-    aError.Throw(NS_ERROR_INVALID_POINTER);
>-    return;
>+    if (!window || !window->GetExtantDoc()) {
>+      aError.Throw(NS_ERROR_INVALID_POINTER);
>+      return;
>+    }

"!window ||" is redundant.

>+++ b/js/xpconnect/src/xpcprivate.h
>@@ -3596,16 +3596,17 @@ IsSandbox(JSObject *obj);
>         bool XMLHttpRequest;
>         bool TextDecoder;
>         bool TextEncoder;
>+        bool URL;
>         bool atob;
>         bool btoa;

Could you make these bool variables to bitfields to save space?

Comment 4

[Andrea Marchesini [:baku]]

> Am I missing something?  You're not running this test in a worker.

Yes I do. It's included in the Makefile.in.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch

emk, can you take a look at this patch again? Thanks!

https://tbpl.mozilla.org/?tree=Try&rev=a80e5fd7fc97

Comment 6

[Masatoshi Kimura [:emk]]

Comment on attachment 809849 [details] [diff] [review]
patch

Are those bunch of renames needed? It's very likely to break add-ons...

>+++ b/dom/base/URL.cpp
>@@ -15,71 +15,65 @@
>-  if (!window) {
>-    aRv.Throw(NS_ERROR_UNEXPECTED);
>-    return nullptr;
>-  }
>+  nsCOMPtr<nsISupports> parent = do_QueryInterface(aGlobal.GetAsSupports());
>+  // Note: we can be window-less so parent can be NULL.

It cannot. Please leave the null check.

>@@ -138,64 +132,79 @@ URL::CreateObjectURL(const GlobalObject&
>+  if (window) {
>+    NS_PRECONDITION(!window || window->IsInnerWindow(),
>+                    "Should be inner window");

>+    if (!window || !window->GetExtantDoc()) {
>+      aError.Throw(NS_ERROR_INVALID_POINTER);
>+      return;
>+    }

>+  if (window) {
>+    NS_PRECONDITION(!window || window->IsInnerWindow(),
>+                    "Should be inner window");

Please explain the reason if you think these redundant "!window" checks are needed.

>diff --git a/dom/workers/test/test_url.xul b/dom/workers/test/test_url.xul
>new file mode 100644
>--- /dev/null
>+++ b/dom/workers/test/test_url.xul
>@@ -0,0 +1,36 @@
>+    function test()
>+    {
>+      waitForWorkerFinish();
>+
>+      Components.utils.import("chrome://mochitests/content/chrome/dom/workers/test/file_url.jsm");
>+      checkFromJSM(ok, is, finish);
>+    }

Is checkFromJSM() executed inside a worker thread? If not, what's the difference from dom/base/test/test_url.xul?

Comment 7

[Andrea Marchesini [:baku]]

> Are those bunch of renames needed? It's very likely to break add-ons...

Right. But unfortunately if URL is global, const URL = 'foobar' throws. It's green on try... but i don't know if this will break addons.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: url.patch

Comment 9

[Masatoshi Kimura [:emk]]

Comment on attachment 809920 [details] [diff] [review]
url.patch

+++ b/dom/base/URL.cpp
@@ -138,64 +138,78 @@ URL::CreateObjectURL(const GlobalObject&
>+  if (window) {
>+    NS_PRECONDITION(!window || window->IsInnerWindow(),
>+                    "Should be inner window");

f=me with removing this redundant !window chack (in URL::RevokeObjectURL).

Comment 10

[Andrea Marchesini [:baku]]

Attachment: url.patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

> const URL = 'foobar' throws.

I claim this is a bug in spidermonkey.  We should just fix it...

Comment 12

[Masatoshi Kimura [:emk]]

(In reply to Boris Zbarsky [:bz] from comment #11)
> > const URL = 'foobar' throws.
> 
> I claim this is a bug in spidermonkey.  We should just fix it...

Bug 611388 will fix this because const will no longer be a property of the global object, but I don't think it will be fixed in near future.

Comment 13

[Tom S [:evilpie]]

I think for a short term fix we could just accept redefinition of properties on the global object in here: http://mxr.mozilla.org/mozilla-central/source/js/src/vm/Interpreter-inl.h#263.

Comment 14

[(no longer active)]

Wait.  I don't think this const URL bug is going to be acceptable.  This will bite front-end developers, and add-on developers as well.

Unfortunately I cannot see <https://mxr.mozilla.org/addons/search?string=const%2BURL> right now so I can't prove my theory, but we should err on the side of caution here.

Andrea, can you see if you can take a stab at comment 13?  Or get Tom to do that for you?  :-)

Comment 15

[(no longer active)]

Also, given bug 920179, we should get Bobby's blessing here.

Comment 16

[Masatoshi Kimura [:emk]]

Then
  var foo = 1;
  const foo = 2;
  const foo = 42;
will no longer throw? I don't think it is correct.

Comment 17

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #14)
> Andrea, can you see if you can take a stab at comment 13?  Or get Tom to do
> that for you?  :-)

Let's do bug 920553 instead, unless anyone has any beefs with that approach.

Comment 18

[Andrea Marchesini [:baku]]

Attachment: url.patch

This patch uses what has been implemented by bug 920553.

Comment 19

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=4292308655bf

Comment 20

[(no longer active)]

Comment on attachment 812048 [details] [diff] [review]
url.patch

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

Looks good.  Bobby, can you take a look at this as well?

Thanks!

::: dom/base/URL.cpp
@@ +40,5 @@
>  /* static */ already_AddRefed<URL>
>  URL::Constructor(const GlobalObject& aGlobal, const nsAString& aUrl,
>                   URL& aBase, ErrorResult& aRv)
>  {
> +  nsCOMPtr<nsISupports> parent = do_QueryInterface(aGlobal.GetAsSupports());

You don't need to QI an nsISupports to nsISupports.  :-)

@@ +69,5 @@
>  /* static */ already_AddRefed<URL>
>  URL::Constructor(const GlobalObject& aGlobal, const nsAString& aUrl,
>                    const nsAString& aBase, ErrorResult& aRv)
>  {
> +  nsCOMPtr<nsISupports> parent = do_QueryInterface(aGlobal.GetAsSupports());

Same here.

Comment 21

[Bobby Holley (:bholley)]

Comment on attachment 812048 [details] [diff] [review]
url.patch

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

Please add additional tests to verify that this stuff works for Sandboxes - see js/xpconnect/tests/unit/test_textDecoder.js for an example.

::: dom/base/URL.cpp
@@ +41,5 @@
>  URL::Constructor(const GlobalObject& aGlobal, const nsAString& aUrl,
>                   URL& aBase, ErrorResult& aRv)
>  {
> +  nsCOMPtr<nsISupports> parent = do_QueryInterface(aGlobal.GetAsSupports());
> +  if (!parent) {

This is going to break for Sandboxes, which don't use native globals.

@@ +158,5 @@
> +
> +    doc = window->GetExtantDoc();
> +    principal = doc->NodePrincipal();
> +  } else {
> +    principal = nsContentUtils::GetSystemPrincipal();

No! Defaulting to the system principal is exactly the sort of thing that security bugs are made of.

Please just either use nsContentUtils::GetObjectPrincipal on the global, or enter the global's compartment and use nsContentUtils::GetSystemPrincipal.

Comment 22

[Andrea Marchesini [:baku]]

> > +  nsCOMPtr<nsISupports> parent = do_QueryInterface(aGlobal.GetAsSupports());
> > +  if (!parent) {
> 
> This is going to break for Sandboxes, which don't use native globals.

It passes the test. parent is not null. Do I miss something?
Soon a patch with the other comments applied.

Comment 23

[Bobby Holley (:bholley)]

(In reply to Andrea Marchesini (:baku) from comment #22)
> > > +  nsCOMPtr<nsISupports> parent = do_QueryInterface(aGlobal.GetAsSupports());
> > > +  if (!parent) {
> > 
> > This is going to break for Sandboxes, which don't use native globals.
> 
> It passes the test.

What test? I don't see any tests in this patch. Or did you add one since then?

> parent is not null. Do I miss something?

Actually, because gabor recently put an nsISupports private in sandboxes, this will actually be treated as a slim wrapper by xpc_qsUnwrapArgImpl (!!!), and cause AsSupports to return non-null. But SandboxPrivate is _very_ different from a wrapped global, and we shouldn't be conflating the two.

And anyway, we should always pull the principal directly from the JS global if we can.

Comment 24

[Andrea Marchesini [:baku]]

Attachment: patch

This is the test I added. Let me know if what I do with nsIPrincipal is ok.

Comment 25

[Bobby Holley (:bholley)]

Comment on attachment 812869 [details] [diff] [review]
patch

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

::: dom/base/URL.cpp
@@ +40,5 @@
>  /* static */ already_AddRefed<URL>
>  URL::Constructor(const GlobalObject& aGlobal, const nsAString& aUrl,
>                   URL& aBase, ErrorResult& aRv)
>  {
> +  nsCOMPtr<nsISupports> parent = aGlobal.GetAsSupports();

My point is that we should be calling aGlobal.GetAsSupports() here, because it only works for Sandboxes by virtue of a bug in quickstubs unwrapping. We should just use the global as a JSObject.

Comment 26

[Andrea Marchesini [:baku]]

Attachment: url.patch

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 813752 [details] [diff] [review]
url.patch

This generally looks good.  The one worry I have is that this is not giving non-document scopes the auto-revoking behavior that documents have.  Is it worth filing a followup to look into a way of doing htat?

Comment 28

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=75d91b49e777

if green, we can land it.

Comment 29

[Bobby Holley (:bholley)]

Comment on attachment 813752 [details] [diff] [review]
url.patch

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

::: dom/base/URL.cpp
@@ +145,5 @@
> +
> +    doc = window->GetExtantDoc();
> +    principal = doc->NodePrincipal();
> +  } else {
> +    principal = nsContentUtils::GetObjectPrincipal(aGlobal.Get());

Unless I'm missing something, we should unconditionally use the nContentUtils::GetObjectPrincipal path. What does the |window| path buy us?

@@ +178,5 @@
> +                    "Should be inner window");
> +    principal = window->GetPrincipal();
> +  } else {
> +    principal = nsContentUtils::GetObjectPrincipal(aGlobal.Get());
> +  }

Same here.

Comment 30

[Andrea Marchesini [:baku]]

Attachment: url.patch

Comment 31

[Bobby Holley (:bholley)]

Comment on attachment 813774 [details] [diff] [review]
url.patch

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

::: dom/base/URL.cpp
@@ +130,5 @@
>                               const objectURLOptions& aOptions,
>                               nsString& aResult, ErrorResult& aError)
>  {
> +  nsCOMPtr<nsIPrincipal> principal = nsContentUtils::GetObjectPrincipal(aGlobal.Get());
> +  if (!principal) {

GetObjectPrincipal will never return null.

@@ +166,5 @@
>  void
>  URL::RevokeObjectURL(const GlobalObject& aGlobal, const nsAString& aURL)
>  {
> +  nsIPrincipal* principal = nsContentUtils::GetObjectPrincipal(aGlobal.Get());
> +  if (!principal) {

same here.

::: dom/base/URL.h
@@ +42,2 @@
>    {
> +    return nullptr;

So, this is what I wanted to talk to bz about.

With this approach, we'll basically need to return |nullptr| from GetParentObject for every DOM object whose constructors are exposed to non-window globals. Is that ok? It seems like it sort of depends on where |aScope| comes from. If it comes from the global object of the associated constructor, that should work fine. But if it somehow comes from |cx|, then it seems like we could run into trouble with Xrays somehow.

Put another way - is it ok for a wide swath of C++ DOM objects not to have a native reference to their global object? Or should we make something work with nsIGlobalObject somehow?

Comment 32

[Boris Zbarsky [:bzbarsky]]

For the specific case of WebIDL constructors, aScope is the callee constructor function, unless that happens to be an Xray, in which case it's the thing the Xray is wrapping.  That is, it's the actual constructor function from the right scope, in all cases.  Then the bindings code finds the global of aScope.

If there were a way to get at URL objects other than just the constructor, we might need to worry anyway (in case the wrapper got collected), but I don't think there is such a way right now.

Comment 33

[Andrea Marchesini [:baku]]

> GetObjectPrincipal will never return null.

I removed these checks. Do you have any additional comment or can I land this patch? Thanks!

Comment 34

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #32)
> If there were a way to get at URL objects other than just the constructor,
> we might need to worry anyway (in case the wrapper got collected), but I
> don't think there is such a way right now.

Well, I'm interested in solving this problem in general, not just for URL. Every constructor that we want to expose outside of Window scopes is going to have this problem. Are there likely to be overlapping cases where we want to expose the constructor outside of Document/Window-land, but there are also paths to obtain an instance that might have been collected?

Comment 35

[Bobby Holley (:bholley)]

Comment on attachment 813774 [details] [diff] [review]
url.patch

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

r=bholley with comments addressed.

::: dom/base/URL.cpp
@@ +19,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
> +NS_IMPL_CYCLE_COLLECTION_1(URL, mURI)

Do we really need to cycle-collect nsIURI? We weren't before - is the issue that the traversal of the window was always sufficient?

@@ +179,3 @@
>    bool subsumes;
> +  if (urlPrincipal && principal &&
> +      NS_SUCCEEDED(principal->Subsumes(urlPrincipal, &subsumes)) &&

You shouldn't need the out-param for Subsumes - I added an overload that returns a boolean directly.

Comment 36

[Andrea Marchesini [:baku]]

> > +NS_IMPL_CYCLE_COLLECTION_1(URL, mURI)
> 
> Do we really need to cycle-collect nsIURI? We weren't before - is the issue
> that the traversal of the window was always sufficient?

I suspect I need to traverse mURI. I use nsCOMPtr<nsIURI> and this uri is generated by an nsIIOService with NewURI().
bz?

Comment 37

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

AFAIK we don't have any nsIURI implementation which is cycle collectable. And if so, there is no
need to CC mURI.

Comment 38

[Boris Zbarsky [:bzbarsky]]

> Are there likely to be overlapping cases where we want to expose the constructor outside
> of Document/Window-land, but there are also paths to obtain an instance that might have
> been collected?

I don't know offhand...  Event?

> AFAIK we don't have any nsIURI implementation which is cycle collectable.

Fwiw, nothing stops an extension that implements a protocol handler from implementing nsIURI in JS.  I really hope none do, of course.

Comment 39

[Andrea Marchesini [:baku]]

> Fwiw, nothing stops an extension that implements a protocol handler from
> implementing nsIURI in JS.  I really hope none do, of course.

This is what I meant. I think that if there is at least 1 possibility to have cycle collectable nsIURI, we should keep this implementation. how does it sound?

Comment 40

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

Hmm, indeed nsIURI can be implemented in JS, or binary addons may do something unexpected.
However, in general we don't traverse/unlink nsIURI objects.
Safest option would be to traverse/unlink uris always, but such change could be done in a followup which goes through all the CC impls.

Comment 41

[Masatoshi Kimura [:emk]]

Or, what about making nsIURI [builtinclass]? It will also help to implement bug 906714.

Comment 42

[Andrea Marchesini [:baku]]

Attachment: url.patch

Comment 43

[Andrea Marchesini [:baku]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/977afa826c5e

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/977afa826c5e

Comment 45

[Masatoshi Kimura [:emk]]

Cu.importGlobalProperties should be officially documented because :dustin thought this was a test-only method (see bug 803188).