Bob Owen (:bobowen) Assignee
	Description • 11 years ago

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.

	Bobby Holley (:bholley)
	Comment 1 • 11 years ago

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.

	Bob Owen (:bobowen) Assignee
	Comment 2 • 11 years ago

(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.

	Bob Owen (:bobowen) Assignee
	Comment 3 • 11 years ago

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.

	Bob Owen (:bobowen) Assignee
	Comment 4 • 11 years ago

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

	Bob Owen (:bobowen) Assignee
	Comment 5 • 11 years ago

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

	Bob Owen (:bobowen) Assignee
	Comment 6 • 11 years ago

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.

	Bobby Holley (:bholley)
	Comment 7 • 11 years ago

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.

	Bob Owen (:bobowen) Assignee
	Comment 8 • 11 years ago

(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?

	Bobby Holley (:bholley)
	Comment 9 • 11 years ago

(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.

	Bob Owen (:bobowen) Assignee
	Comment 10 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 11 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 12 • 11 years ago

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