Closed Bug 1069719 Opened 5 years ago Closed 5 years ago

Implement CheckSafetyInPrerendering

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: rvid, Assigned: ehsan)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
Blocks: prerendering
Attached patch 1069719.patch(WIP) (obsolete) — Splinter Review
Note that bug 1119866 will improve the error message emitted here.
Depends on: 1119866
Comment on attachment 8497843 [details] [diff] [review]
1069719.patch(WIP)

I decided against doing things this way, I think having a separate interrupt handler makes the code simpler.  We also need the abortion part to be reusable for things that cannot be blacklisted in the IDL, and my patch adds a function called HandlePrerenderingViolation() for that purpose.
Attachment #8497843 - Attachment is obsolete: true
Assignee: nobody → ehsan
Depends on: 1120424
What guarantees that the interrupt you get will be for the prerendering script while the runtime callback is swizzled?
Flags: needinfo?(ehsan)
(In reply to Boris Zbarsky [:bz] from comment #5)
> What guarantees that the interrupt you get will be for the prerendering
> script while the runtime callback is swizzled?

Hmm, I thought that is guaranteed by the JS_RequestInterruptCallback API.  Is it not?  Is there a better way to do this?
Flags: needinfo?(ehsan) → needinfo?(bzbarsky)
I'm not sure what JS_RequestInterruptCallback really guarantees.  For example, if the call that does the swizzling is the last jsop in the script, will we really get the interrupt before the script is done?

Based on the docs, certainly nothing guarantees the callback any particular time after the request.

> Is there a better way to do this?

Thinking about it.
CheckForInterrupt seems to be called from all over the place <https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22js%3A%3ACheckForInterrupt%28struct+JSContext+*%29%22>.

Waldo, can you please answer the question in comment 7?  Or point us to someone who knows the semantics of this API?  Thanks!
Flags: needinfo?(jwalden+bmo)
(Also the comments around <https://dxr.mozilla.org/mozilla-central/source/js/src/asmjs/AsmJSSignalHandlers.cpp#1065> seem to be relevant.)
> Is there a better way to do this?

So in terms of bindings, can we not just return false without setting a pending exception if an unsafe API is called while in prerendering?  That creates an uncatchable exception, which seems like the desired goal, right?

This should even work for promise-returning methods; the code that converts exceptions to rejections just returns false if there is no pending exception.

For cases that want to do the check in the C++ below the binding level, we'd probably just want to add a way to throw an uncatchable exception on an ErrorResult.  We've talked about that on and off anyway, and it's pretty easy to do.

Thoughts?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #10)
> > Is there a better way to do this?
> 
> So in terms of bindings, can we not just return false without setting a
> pending exception if an unsafe API is called while in prerendering?  That
> creates an uncatchable exception, which seems like the desired goal, right?

Sorry, return false where?

> This should even work for promise-returning methods; the code that converts
> exceptions to rejections just returns false if there is no pending exception.
> 
> For cases that want to do the check in the C++ below the binding level, we'd
> probably just want to add a way to throw an uncatchable exception on an
> ErrorResult.  We've talked about that on and off anyway, and it's pretty
> easy to do.
> 
> Thoughts?

What you're saying makes general sense but I'd like to understand the exact solution a bit better.
Flags: needinfo?(bzbarsky)
> Sorry, return false where?

From the JSNative (the generated bindings code) to the JS engine.
Flags: needinfo?(bzbarsky)
Comment on attachment 8547583 [details] [diff] [review]
Abort the execution of scripts when a prerendered page calls an IDL blacklisted function

I'm pretty sure this does not work...
Attachment #8547583 - Flags: review?(bzbarsky) → review-
Attachment #8547583 - Attachment is obsolete: true
Flags: needinfo?(jwalden+bmo)
Comment on attachment 8551448 [details] [diff] [review]
Abort the execution of scripts when a prerendered page calls an IDL blacklisted function

Err, I split this patch by mistake...
Attachment #8551448 - Attachment is obsolete: true
Attachment #8551448 - Flags: review?(bzbarsky)
Comment on attachment 8551467 [details] [diff] [review]
Abort the execution of scripts when a prerendered page calls an IDL blacklisted function

>CheckSafetyInPrerendering(JSContext* aCx, JSObject* aObj)

So I have a few issues with this name and signature:

1)  aObj is unused.  Can we just drop it?
2)  This is actually checking whether the _caller_ is in prerendering, not the
    _callee_, right?  So if we happen to access an unsafe API in a prerendered
    thing via Xrays this function will return false.  Are we OK with that?  I
    guess that's better than killing the script that's doing the Xray access,
    but if the API is unsafe...
3)  The name should match what the function is doing, once we decide that that
    is.

>+WindowGlobalOrSandboxedGlobalOrNull(JSObject *aObj)

I'm not sure this function makes sense for your use case, but I'd like to understand what the exact setup is for the inner/outer window during prerendering.  I assume the current inner for the toplevel window is the non-prerendering inner, right?

The problem is that the proto of the sandbox is the _outer_, no?  So in the sandbox case, this will only return a window in prerendering for subframes, not for the toplevel window.  Is that the behavior you were expecting?  At the very least, this needs some really careful documentation.

>+ * Otherwise if it's a sandboxed object (which would be the case for GreaseMonkey

s/sandboxed/sandbox/ and again we need to really carefully document which exact nsGlobalWindow is returned here.
Attachment #8551467 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky [:bz] from comment #17)
> Comment on attachment 8551467 [details] [diff] [review]
> Abort the execution of scripts when a prerendered page calls an IDL
> blacklisted function
> 
> >CheckSafetyInPrerendering(JSContext* aCx, JSObject* aObj)
> 
> So I have a few issues with this name and signature:
> 
> 1)  aObj is unused.  Can we just drop it?

Yes.

> 2)  This is actually checking whether the _caller_ is in prerendering, not
> the
>     _callee_, right?  So if we happen to access an unsafe API in a
> prerendered
>     thing via Xrays this function will return false.  Are we OK with that?  I
>     guess that's better than killing the script that's doing the Xray access,
>     but if the API is unsafe...

Oh, no, that's not OK.  We should check the callee here, I think.  How can I get a JSContext*/nsPIDOMWindow associated with the callee though?

> 3)  The name should match what the function is doing, once we decide that
> that
>     is.
> 
> >+WindowGlobalOrSandboxedGlobalOrNull(JSObject *aObj)
> 
> I'm not sure this function makes sense for your use case, but I'd like to
> understand what the exact setup is for the inner/outer window during
> prerendering.  I assume the current inner for the toplevel window is the
> non-prerendering inner, right?

Hmm, no, it should be a prerendering inner.

> The problem is that the proto of the sandbox is the _outer_, no?  So in the
> sandbox case, this will only return a window in prerendering for subframes,
> not for the toplevel window.  Is that the behavior you were expecting?  At
> the very least, this needs some really careful documentation.

Oh no, not at all.  I think I'm heavily misunderstanding this whole setup.

Come to think of it, do I need to care about the sandbox case at all here?  Perhaps I was just going down a rathole?
> How can I get a JSContext*/nsPIDOMWindow associated with the callee though?

I think you have two main options.

1)

  {
    JSObject* unwrapped = js::CheckedUnwrap(obj);
    // Handle unwrapped being null however you want; it's about to throw anyway
    nsGlobalWindow* win = xpc::WindowGlobalOrNull(unwrapped);
  }

This will base the decision on the "this" object.  As long as you're sure that the "this" object and the code doing the call are both in prerendering or both not in prerendering in the case of web calls, this should be ok, I'd think.

2)  Use xpc::XrayAwareCalleeGlobal on the callee, then xpc::WindowOrNull.  This bases the decision on the function being called instead of the "this" object.

I think in practice both would end up being equivalent as long as code in a prerendering window can't get its hands on objects outside such a window and vice versa.  Performance wise, I think they're approximately the same.

I do think that in the Xray case we should report something to the browser console in addition to killing the script or something...

> Come to think of it, do I need to care about the sandbox case at all here?

That basically depends on whether greasemonkey scripts and the like can run on the prerendering, right?  I think we should strive to have them NOT do so, because having them run on a non-current inner seems like a recipe for all sorts of confusion.
(In reply to Boris Zbarsky [:bz] from comment #19)
> > How can I get a JSContext*/nsPIDOMWindow associated with the callee though?
> 
> I think you have two main options.
> 
> 1)
> 
>   {
>     JSObject* unwrapped = js::CheckedUnwrap(obj);
>     // Handle unwrapped being null however you want; it's about to throw
> anyway
>     nsGlobalWindow* win = xpc::WindowGlobalOrNull(unwrapped);
>   }
> 
> This will base the decision on the "this" object.  As long as you're sure
> that the "this" object and the code doing the call are both in prerendering
> or both not in prerendering in the case of web calls, this should be ok, I'd
> think.
> 
> 2)  Use xpc::XrayAwareCalleeGlobal on the callee, then xpc::WindowOrNull. 
> This bases the decision on the function being called instead of the "this"
> object.
> 
> I think in practice both would end up being equivalent as long as code in a
> prerendering window can't get its hands on objects outside such a window and
> vice versa.  Performance wise, I think they're approximately the same.

Thanks!  I'll give this a shot tomorrow.

> I do think that in the Xray case we should report something to the browser
> console in addition to killing the script or something...

We need to do that in any case, I think.  I'll do that in bug 1119866.  I don't yet have a good plan on where to report this.  The right place may depend on how prerendering was triggered (i.e., whether it was triggered from the page or from the UA.)

> > Come to think of it, do I need to care about the sandbox case at all here?
> 
> That basically depends on whether greasemonkey scripts and the like can run
> on the prerendering, right?  I think we should strive to have them NOT do
> so, because having them run on a non-current inner seems like a recipe for
> all sorts of confusion.

But do we have a way to prevent that from happening?
> But do we have a way to prevent that from happening?

I don't know.
Comment on attachment 8551467 [details] [diff] [review]
Abort the execution of scripts when a prerendered page calls an IDL blacklisted function

I have something based on (1) in comment 19 which seems to work well.
Attachment #8551467 - Attachment is obsolete: true
Comment on attachment 8553949 [details] [diff] [review]
Abort the execution of scripts when a prerendered page calls an IDL blacklisted function

Thanks, this is much better.

I'm still not a fan of the CheckSafetyInPrerendering in prerendering name.  It doesn't indicate anything about the side-effects of calling this function (in particular the HandlePrerenderingViolation call).

How does this sound: rename the function to EnforceSafetyInPrerendering (or maybe EnsureNotInPrerendering or EnforceNotInPrerendering or something?) and reverse the return value, so false means "bail out".  Then on the caller side it looks like a normal "hey, we called this thing, it failed, propagate the failure" kind of thing.  And we add a comment in the function about how we're purposefully returning false without throwing to produce an uncatchable exception.

r=me with something like the above...
Attachment #8553949 - Flags: review?(bzbarsky) → review+
Either this or bug 1125351 is causing mochitest-other failures. Please fix or backout ASAP. Inbound is closed until then.
https://treeherder.mozilla.org/logviewer.html#?job_id=5870479&repo=mozilla-inbound
Flags: needinfo?(ehsan)
Sorry I wasn't around...  This turned out to be bug 1125351.  I fixed that issue, and relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cfdd4c2d83b
Flags: needinfo?(ehsan)
https://hg.mozilla.org/mozilla-central/rev/3cfdd4c2d83b
https://hg.mozilla.org/mozilla-central/rev/ad130e07458d
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Duplicate of this bug: 1231753
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.