Closed Bug 920015 Opened 7 years ago Closed 7 years ago

Expose DOM URL to js modules

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: Yoric, Assigned: baku)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 8 obsolete files)

Could we access global "URL" from js modules? Pretty please?
Attached patch patch (obsolete) — Splinter Review
Attachment #809213 - Flags: review?(ehsan)
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.
Attachment #809213 - Flags: review?(ehsan) → review+
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?
> Am I missing something?  You're not running this test in a worker.

Yes I do. It's included in the Makefile.in.
Attached patch patch (obsolete) — Splinter Review
emk, can you take a look at this patch again? Thanks!

https://tbpl.mozilla.org/?tree=Try&rev=a80e5fd7fc97
Attachment #809213 - Attachment is obsolete: true
Attachment #809849 - Flags: feedback?(VYV03354)
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?
> 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.
Attached patch url.patch (obsolete) — Splinter Review
Attachment #809849 - Attachment is obsolete: true
Attachment #809849 - Flags: feedback?(VYV03354)
Attachment #809920 - Flags: feedback?(VYV03354)
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).
Attachment #809920 - Flags: feedback?(VYV03354) → feedback+
Attached patch url.patch (obsolete) — Splinter Review
Attachment #809920 - Attachment is obsolete: true
Keywords: checkin-needed
> const URL = 'foobar' throws.

I claim this is a bug in spidermonkey.  We should just fix it...
Flags: needinfo?(evilpies)
(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.
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.
Flags: needinfo?(evilpies)
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?  :-)
Keywords: checkin-needed
Also, given bug 920179, we should get Bobby's blessing here.
Then
  var foo = 1;
  const foo = 2;
  const foo = 42;
will no longer throw? I don't think it is correct.
(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.
Attached patch url.patch (obsolete) — Splinter Review
This patch uses what has been implemented by bug 920553.
Attachment #809937 - Attachment is obsolete: true
Attachment #812048 - Flags: review?(ehsan)
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.
Attachment #812048 - Flags: review?(ehsan)
Attachment #812048 - Flags: review?(bobbyholley+bmo)
Attachment #812048 - Flags: review+
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.
Attachment #812048 - Flags: review?(bobbyholley+bmo) → review-
> > +  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.
(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.
Attached patch patch (obsolete) — Splinter Review
This is the test I added. Let me know if what I do with nsIPrincipal is ok.
Attachment #812048 - Attachment is obsolete: true
Attachment #812869 - Flags: review?(bobbyholley+bmo)
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.
Attachment #812869 - Flags: review?(bobbyholley+bmo) → review-
Attached patch url.patch (obsolete) — Splinter Review
Attachment #812869 - Attachment is obsolete: true
Attachment #813752 - Flags: review?(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?
Attachment #813752 - Flags: review?(bzbarsky)
Attachment #813752 - Flags: review+
Attachment #813752 - Flags: feedback?(khuey)
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.
Attached patch url.patch (obsolete) — Splinter Review
Attachment #813752 - Attachment is obsolete: true
Attachment #813752 - Flags: feedback?(khuey)
Attachment #813774 - Flags: review?(bobbyholley+bmo)
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?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
> GetObjectPrincipal will never return null.

I removed these checks. Do you have any additional comment or can I land this patch? Thanks!
Flags: needinfo?(bobbyholley+bmo)
(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?
Flags: needinfo?(bzbarsky)
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.
Attachment #813774 - Flags: review?(bobbyholley+bmo) → review+
Flags: needinfo?(bobbyholley+bmo)
> > +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?
AFAIK we don't have any nsIURI implementation which is cycle collectable. And if so, there is no
need to CC mURI.
> 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.
Flags: needinfo?(bzbarsky)
> 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?
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
Or, what about making nsIURI [builtinclass]? It will also help to implement bug 906714.
Attached patch url.patchSplinter Review
Attachment #813774 - Attachment is obsolete: true
Attachment #814873 - Flags: review?(bugs)
Attachment #814873 - Flags: review?(bugs) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/977afa826c5e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Cu.importGlobalProperties should be officially documented because :dustin thought this was a test-only method (see bug 803188).
Keywords: dev-doc-needed
Depends on: 1156875
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.