944363 - Consistency for how iframe sandboxing propagates errors on blocking.

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Bob Owen (:bobowen)]

Currently when navigation is blocked by iframe sandboxing, there seems to be some inconsistency with the way that errors are propagated.

window.open, location.replace, location.assign, and location.href all propagate when blocked.
Most other property set on location (hash, pathname, etc.) do not.
This is because the rv from nsLocation::SetURI gets dropped for all of these.
Understandably anchor clicks do not propagate.

The spec seems unclear on what the desired behaviour should be.

webkit appears not to throw for any of these (or at least the ones I've tested).
I thought that it did, but it just puts out a console message without throwing.

Comment 1

[Bobby Holley (:bholley)]

If the spec gives us no guidance, I think we should land a patch to propagate the error, and file a spec bug on sorting it out.

Comment 2

[Bob Owen (:bobowen)]

(In reply to Bobby Holley (:bholley) from comment #1)
> If the spec gives us no guidance, I think we should land a patch to
> propagate the error, and file a spec bug on sorting it out.

OK, just had another look at the spec and it just says "abort these steps", but not whether to throw an exception.
The text that "calls" the navigate algorithm doesn't mention anything about what to do if aborted either.

Propagating errors should be pretty straight forward.
We should just need to capture and return rv from nsLocation::SetURI

The new tests for bug 785310, would just need switching to expect an exception.

I won't pick this up as part of bug 785310, so it will be easy to back it out separately if necessary.

Comment 3

[Bob Owen (:bobowen)]

Attachment: Change sandboxed navigation by location tests to expect excpetions.

This is for hash and pathname.
Also block only tests for host, hostname, port, protocol and search.

I should know better than using the word "just" in comment 2, the tests took a little bit more work than I thought.

Comment 4

[Bob Owen (:bobowen)]

Attachment: Change functions that call SetURI in nsLocation to propagate return values.

These two patches build on top of patches for bug 785310.

I'll get the bug raised for the spec or I might email whatwg.

try push: https://tbpl.mozilla.org/?tree=Try&rev=9fc29fe29885

Comment 5

[Bob Owen (:bobowen)]

Spec bug filed: https://www.w3.org/Bugs/Public/show_bug.cgi?id=24110

Comment 6

[Bob Owen (:bobowen)]

Attachment: Bug 944363 fix V2 - Change functions that call SetURI in nsLocation to propagate return values.

Merged with mozilla-central.

The tests in bug 785310 now allow for and test these changes.
So, this bug must be landed at the same time as bug 785310.

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8359083 [details] [diff] [review]
Bug 944363 fix V2 - Change functions that call SetURI in nsLocation to propagate return values.

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

I just commented on the first two chunks, and then noticed that the rest is analogous - can you fix them all up and then re-request review?

::: dom/base/nsLocation.cpp
@@ +312,5 @@
>      hash.Insert(char16_t('#'), 0);
>    }
>    rv = uri->SetRef(hash);
>    if (NS_SUCCEEDED(rv)) {
> +    rv = SetURI(uri);

Let's change this to:

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}
return SetURI(uri);

@@ +352,4 @@
>      return NS_ERROR_DOM_SECURITY_ERR;
>  
>    nsCOMPtr<nsIURI> uri;
>    nsresult rv = GetWritableURI(getter_AddRefs(uri));

Let's check-and-return here to remove the conditional that follows:

if (NS_WARN_IF(NS_FAILED(rv) || !uri))) {
  return rv;
}

(Note that in previous days this would be an NS_ENSURE_*, but per a recent dev-platform announcement, that style is now forbidden).

and then make the rest of this function look something along the lines of what I suggested above.

Comment 8

[Bob Owen (:bobowen)]

Attachment: Bug 944363 fix V3 - Change functions that call SetURI in nsLocation to propagate return values.

(In reply to Bobby Holley (:bholley) from comment #7)
> Comment on attachment 8359083 [details] [diff] [review]

Thanks bholley, I think I've made all the changes as requested.

New try push (combined with bug 785310):
https://tbpl.mozilla.org/?tree=Try&rev=528ee18ab10e

> (Note that in previous days this would be an NS_ENSURE_*, but per a recent
> dev-platform announcement, that style is now forbidden).

In my patch for bug 785310, I've used NS_ENSURE_SUCCESS(rv, rv), does this need changing to something else now?

Comment 9

[Bobby Holley (:bholley)]

(In reply to Bob Owen (:bobowen) from comment #8)
> In my patch for bug 785310, I've used NS_ENSURE_SUCCESS(rv, rv), does this
> need changing to something else now?

Nah it's fine. It's a gradual transition, probably with an automatic component at some point.

Comment 10

[Bob Owen (:bobowen)]

Needs to be landed with fix and tests patch for bug 785310.

https://bugzilla.mozilla.org/attachment.cgi?id=8359079
https://bugzilla.mozilla.org/attachment.cgi?id=8361137

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/950acc30d3dc

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/950acc30d3dc