Closed Bug 832014 Opened 7 years ago Closed 6 years ago

Convert Location to WebIDL

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 2 open bugs)

Details

Attachments

(10 files)

40.26 KB, patch
Details | Diff | Splinter Review
14.44 KB, patch
peterv
: review+
Details | Diff | Splinter Review
10.29 KB, patch
peterv
: review+
Details | Diff | Splinter Review
6.36 KB, patch
peterv
: review+
Details | Diff | Splinter Review
46.18 KB, patch
peterv
: review+
Details | Diff | Splinter Review
12.63 KB, patch
peterv
: review+
bholley
: review+
Details | Diff | Splinter Review
8.69 KB, patch
bholley
: review+
Details | Diff | Splinter Review
6.35 KB, patch
peterv
: review+
Details | Diff | Splinter Review
13.36 KB, patch
peterv
: review+
bholley
: review+
Details | Diff | Splinter Review
2.68 KB, patch
bholley
: review+
Details | Diff | Splinter Review
This needs some wrappers/security work first.
Depends on: 658909
Blocks: 875198
(In reply to Boris Zbarsky (:bz) from comment #0)
> This needs some wrappers/security work first.

Still? If so, what?
Bobby?
Flags: needinfo?(bobbyholley+bmo)
The XPCWN situation is now in a good place for this, but we still need a WebIDL equivalent of UnwrapThisIfAllowed.

Basically, to make call/apply work, cross-origin-accessible methods need to be able to do an unsafe unwrap of security wrappers. The simplest thing from a bindings perspective is just to add an annotation to the WebIDL of the method that will cause Codegen to do the right thing. But then we end up storing our ACL in two places (in the WebIDL, and in IsPermitted).

What we could do is to Codegen IsPermitted from WebIDL annotations in some sort of generated BindingUtils. Does that seem reasonable?
Flags: needinfo?(bobbyholley+bmo)
Possibly, yes.

Note that right now the unwrapping code is NOT codegenned, though the calls to it are; we'd need to figure out how to jam the codegen-determined security checks in there.  What form do those take, exactly?
(In reply to Boris Zbarsky (:bz) from comment #4)
> Possibly, yes.
> 
> Note that right now the unwrapping code is NOT codegenned, though the calls
> to it are; we'd need to figure out how to jam the codegen-determined
> security checks in there.  What form do those take, exactly?

Which ones?

The UnwrapThisIfAllowed stuff can be implementing by replacing CheckedUnwrap with UncheckedUnwrap in the codegened stubs for XO-accessible methods.

For IsPermitted, it's probably best to just look at the existing function. We want something that does that, more or less:
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/AccessCheck.cpp#144
The point is there are no CheckedUnwrap calls in the codegen.  The codegen calls some other code, which is what does the unwrap calls.

Futhermore, the call to the code that does the unwrap is in the shared (codegenned) genericMethod code, not in the per-method stubs.

So we'll need to write some sort of entirely new codegen for "these cases".  My question is what "these cases" actually are.
So for example, whether there's a sane way to do it without a separate genericMethod for them.

I guess if it's just annotation in the IDL, there really is not.
Can we just make genericCrossOriginMethod and call that for the right methods?
Sure.  Do we need that for getters and setters too, or just methods?
(In reply to Boris Zbarsky (:bz) from comment #9)
> Sure.  Do we need that for getters and setters too, or just methods?

getters and setters as well.
Depends on: 881886
Attached patch diffSplinter Review
This is the mechanical part of this conversion, or at least the beginning of such. But it does *not* address some of the fundamental missing pieces here, such as the need for Unforgeable or the security wrapper pieces.
For posterity, here's what I recently suggested for the security bits on a private thread:

* Add a new WebIDL extended attribute called [AllowCrossOrigin] that we apply to various properties and methods on Window and Location to indicate that they're cross-origin accessible.
* Use the above to Codegen the IsPermitted policy table that's currently hardcoded in AccessCheck.cpp. This prevents any duplication of sensitive security invariants.
* For [AllowCrossOrigin] properties/methods, we use a special JSNative that uses UncheckedUnwrap instead of CheckedUnwrap to unwrap |this|. This ensures that cross-origin-accessible methods can be call/apply-ed however the caller pleases.
* Add a per-interface attribute called [ExplicitSecurityCheck=Func]. If this is set on an interface, _every_ method that operates the interface (except those marked [AllowCrossOrigin]) must codegen a call to |Func| during this-unwrapping, which is allowed to veto. For Location, Func() will return SubjectPrincipal->Subsumes(PrincipalOfTheActiveInnerForOurOuter). Note the semantics here apply to all parent interfaces, which for us is basically just URLUtils, but the check should only be performed if the object implements an interface that is at least as concrete as the interface for which [ExplicitSecurityCheck] is set. This prevents the security checks from interfering with any other consumers of URLUtils.
Blocks: 957688
No longer blocks: 957688
Blocks: 965898
Notes to self on what needs to be done here:

1)  Need support for unforgeable methods in codegen.
2)  Need support for unforgeable interfaces.  Specifically, need to either require or fake
    a stringifier, force an undefined toJSON, and require or fake a valueOf.  Since this
    interface is likely to be the only unforgeable interface in the platform ever, we
    should just do whatever is easiest for these.
3)  Fix up AccessCheck.
4)  Do the actual conversion.
Assignee: nobody → bzbarsky
Blocks: 996612
Duplicate of this bug: 1021231
Depends on: 1034204
Attachment #8452521 - Flags: review?(peterv)
Attachment #8452521 - Flags: review?(bobbyholley)
Bobby, could you look over the test changes here?
Attachment #8452527 - Flags: review?(peterv)
Attachment #8452527 - Flags: review?(bobbyholley)
Whiteboard: [need review]
Attachment #8452521 - Flags: review?(bobbyholley) → review+
Comment on attachment 8452524 [details] [diff] [review]
part 6.  Make it possible to specify that only a particular interface that implements a given consequential interface gets cross-origin-settable behavior for a particular property

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

r=me modulo those issues.

::: dom/bindings/Codegen.py
@@ +2202,5 @@
>  
> +def IsCrossOriginWritable(attr, descriptor):
> +    """
> +    Return whether the IDLAttribute in question is cross-origin writable on the
> +    interface represented by descriptor.

I'd at least mention Location/URLUtils explicitly here, since that's the only case where this matters (and that's not likely to change).

@@ +2210,5 @@
> +        return False
> +    if crossOriginWritable == True:
> +        return True
> +    assert (isinstance(crossOriginWritable, list) and
> +            len(crossOriginWritable) == 1)

The second part of this assertion seems weird. Why is this a list if we're only planning on it being one entry? Should it be a string? Or alternatively, should we have a comment saying that this assertion reflects the fact that this is only used for Location?
Attachment #8452524 - Flags: review?(bobbyholley) → review+
Comment on attachment 8452527 [details] [diff] [review]
part 8.  Switch Location to WebIDL bindings

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

::: dom/tests/mochitest/bugs/test_bug541530.html
@@ +64,5 @@
>     "shouldn't be able to delete the property off of the prototype");
>  
>  try {
>      this.__defineGetter__.call(Object.getPrototypeOf(window.location), 'href', function(){});
> +    ok(true, "should be able to define things on the prototype");

I'd just nix the try block at this point.

::: js/xpconnect/tests/mochitest/file_bug802557.html
@@ +23,5 @@
>      getLocationApply3: function() {
>        return Function.call.apply(gTS, [loc()]);
>      },
>      getLocationViaPrototype: function() {
> +      return location.toString.call(loc());

This name is no longer correct, and the test duplicates getLocationApply1.

Can you remove this and add a test with URL.prototype.toString (to make sure that the share URLUtils sharing doesn't somehow make that method usable on Location)?

@@ +32,5 @@
>      getHrefViaApply: function() {
>        return Function.call.apply(gGHR, [loc()]);
>      },
>      getHrefViaPrototype: function() {
> +      return Object.getOwnPropertyDescriptor(location, 'href').get.call(loc());

Same here.

::: js/xpconnect/tests/mochitest/test_bug793969.html
@@ +37,5 @@
>  checkThrows(function() { "use strict"; location.valueOf = function() { return {a: 'hah'};} }, 'Shadow with function', /* skipMessageCheck = */ true);
>  checkThrows(function() { Object.defineProperty(location, 'valueOf', { value: function() { return 'hah'; } }); }, 'defineProperty with value');
>  checkThrows(function() { delete location.valueOf; Object.defineProperty(location, 'valueOf', { value: function() { return 'hah'; } }); }, 'delete + defineProperty with value');
> +checkThrows(function() { Object.defineProperty(location, 'valueOf', { get: function() { return 'hah'; } }); }, 'defineProperty with getter');
> +checkThrows(function() { delete location.valueOf; Object.defineProperty(location, 'valueOf', { get: function() { return 'hah'; } }); }, 'delete + defineProperty with getter');

Doh, thanks for fixing those.
Attachment #8452527 - Flags: review?(bobbyholley) → review+
Attachment #8452528 - Flags: review?(bobbyholley) → review+
Blocks: 1036185
> I'd at least mention Location/URLUtils explicitly here

Will do.

> Why is this a list

Because the webidl parser produces a value of True if the extended attribute is used with no value and otherwise a list of the values (typically only one of them).

> I'd just nix the try block at this point.

Will do.

> Can you remove this and add a test with URL.prototype.toString (to make sure that the
> share URLUtils sharing doesn't somehow make that method usable on Location)?

Can do.

> Same here.

OK.
Comment on attachment 8452527 [details] [diff] [review]
part 8.  Switch Location to WebIDL bindings

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

::: dom/tests/mochitest/bugs/test_bug291377.html
@@ +25,5 @@
>  } catch (e) {
>    /* Check that we can touch various properties */
>    isnot(e.lineNumber, undefined, "Unexpected line number"); //This line number is dependent on the implementation of the SpecialPowers API
>    is(e.name, "NS_ERROR_MALFORMED_URI", "Unexpected exception name");
> +  is(e.message, "", "Should have a message for this case");

Should not? And ise, please
> Should not? And ise, please

Done.
Attachment #8452513 - Flags: review?(peterv) → review+
Attachment #8452514 - Flags: review?(peterv) → review+
Comment on attachment 8452517 [details] [diff] [review]
part 3.  Start using nsLocation for the Location interface in bindings

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

We don't want to rename nsLocation to mozilla::dom::Location?
Attachment #8452517 - Flags: review?(peterv) → review+
Attachment #8452518 - Flags: review?(peterv) → review+
Comment on attachment 8452521 [details] [diff] [review]
part 5.  Add WebIDL API to nsLocation

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

::: dom/base/nsLocation.h
@@ +158,2 @@
>    nsWeakPtr mDocShell;
>    nsWeakPtr mOuter;

When would this not be a weak reference to mInnerWindow->GetOuterWindow()? Otherwise we might be able to drop this member?

::: dom/webidl/Location.webidl
@@ +19,2 @@
>    void replace(DOMString url);
> +  // XXXbz there is no forceget argument in the spec!

Bug filed?

@@ +23,2 @@
>  };
> +// No support for .searchParams on Location yet.

Bug filed?
Attachment #8452521 - Flags: review?(peterv) → review+
Attachment #8452526 - Flags: review?(peterv) → review+
Attachment #8452527 - Flags: review?(peterv) → review+
> We don't want to rename nsLocation to mozilla::dom::Location?

Oh, I thought I said something about that.

We have a number of classes called Location in the tree, in other namespaces.  Given how C++ (or maybe our CC macros' use thereof?) treats class name collisions (really really badly; you silently get things like ~Location calling the wrong destructor in some cases!) I decided it was better to leave the name for now.  :(

> When would this not be a weak reference to mInnerWindow->GetOuterWindow()?

Is GetOuterWindow on an inner immutable, even for inners that have been unloaded?  If so, then never, and we can nix mOuter.

> Bug filed?

Good point.  Bug 1037715.
Flags: needinfo?(peterv)
I guess the outer is immutable until the inner is unlinked, right?  Given that, I think we can in fact nix mOuter.
Flags: needinfo?(peterv)
Oh, and filed bug 1037721 for forceGet.
Target Milestone: --- → mozilla33
Comment on attachment 8452528 [details] [diff] [review]
part 9.  Switch AccessCheck to using the generated LocationBinding::IsPermitted method

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

::: js/xpconnect/wrappers/AccessCheck.cpp
@@ +109,1 @@
>          case 'W':

Nit: add a "break;" here to avoid falling through to strcmp(name, "Window").
(In reply to Jan de Mooij [:jandem] from comment #36)
> Nit: add a "break;" here to avoid falling through to strcmp(name, "Window").

I'll handle this in my patches for bug 1036185.
Depends on: 1039187
Depends on: 1041626
Depends on: 1041731
Blocks: 457972
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.