Closed
Bug 944363
Opened 11 years ago
Closed 11 years ago
Consistency for how iframe sandboxing propagates errors on blocking.
Categories
(Core :: DOM: Navigation, defect)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: bobowen, Assigned: bobowen)
References
Details
Attachments
(1 file, 3 obsolete files)
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•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.
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.
Assignee: nobody → bobowencode
Depends on: 785310
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.
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
Assignee | ||
Comment 5•11 years ago
|
||
Spec bug filed: https://www.w3.org/Bugs/Public/show_bug.cgi?id=24110
Assignee | ||
Updated•11 years ago
|
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.
Attachment #8347292 -
Attachment is obsolete: true
Attachment #8347293 -
Attachment is obsolete: true
Attachment #8359083 -
Flags: review?(bobbyholley+bmo)
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.
Attachment #8359083 -
Flags: review?(bobbyholley+bmo)
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?
Attachment #8359083 -
Attachment is obsolete: true
Attachment #8359842 -
Flags: review?(bobbyholley+bmo)
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.
Updated•11 years ago
|
Attachment #8359842 -
Flags: review?(bobbyholley+bmo) → review+
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
Keywords: checkin-needed
Comment 11•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/950acc30d3dc
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/950acc30d3dc
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•