This bug is to track implementing the srcdoc attribute of the iframe element, which HTML5 specifies to allow authors to write the contents of the document in the iframe inline within the parent document. (I'm not expressing an opinion on whether we should do this; it's just the only thing on html5test.com that we didn't have an existing bug report on, so I figured I'd file it.) See: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#attr-iframe-srcdoc http://www.w3.org/TR/html5/the-iframe-element.html#attr-iframe-srcdoc

I do think this is worth doing. I'm happy to mentor someone who wants to work on this, as needed.

I'm interested in working on this. Could you point out which files need to be modified and how to get started?

The right place to start is probably nsFrameLoader.cpp and probably nsHTMLIFrameElement.cpp, which are what decide when to load the src attribute right now. That said, you may need to create some infrastructure in nsDocShell.cpp to do the actual load. Though it's possible that it already has APIs that are good enough for this. What else will need changing depends on what the differences are between scrdoc and just having a data: URI src in terms of behavior, if any.

This converts a srcdoc argument into a data URI. I looked into alternatives, but it appears that it will require a lot of changes as everything expects a URI as an argument. It is currently hardcoded to use charset=UTF-8. Do I need to detect/inherit the appropriate encoding?

Comment on attachment 674213 [details] [diff] [review] patch This gives clearly spec-incorrect behavior for the resulting document. Most importantly, its URI and base URI are wrong, which will break uses of relative URIs in the document. The URI should be an "about:srcdoc" URI. The base URI should be that of the parent document. Note that this means that the child needs to know its a srcdoc document. We also need to know whether a document is a srcdoc document for purposes of determining referrers. And location.reload on a window containing a srcdoc document needs to do something a bit different than a normal reload() call. And srcdoc documents are always in standards mode per spec. Basically, look for "srcdoc document" in http://www.whatwg.org/specs/web-apps/current-work/ I agree that there will probably need to be some core work to make all of this work (in particular, we'll need to communicate the srcdoc document state to the parser to make the standards mode thing work).

Clearing assignment, I've made no progress.

So far I've managed to detect when srcdoc is invoked and then to set the URI to about:srcdoc. As far as I can tell, the next step is to work out how data URIs are handled and then how to call it directly. In this task, I've made no progress. I've looked through nsDocShell, but so far I have been unable to locate where data URIs are treated differently.

When you NS_NewChannel from a data: URI, you get an nsDataChannel. Then you call AsyncOpen on that. But I don't think you necessarily need to pretend like this is a data URI. Have you looked into using the loadStream method on nsIDocShell?

Attached is my progress so far. Very much a work in progress. Reloading and relative links do not work. I've worked out how to use LoadStream and I'm currently calling it from within nsDocShell::LoadURI. My reason for doing so is that LoadStream lacks awareness of referrers, sandboxes etc, which would need to be duplicated. Is this reasonable? I still need to add additional arguments for baseURI and character encoding. Am I correct in saying that the srcdoc iframe inherits the encoding from the parent document?

Hmm. I'd have to look through the code carefully to figure out whether calling LoadStream like that is ok. As for encoding... What does the spec say? On the face of it, scrdoc starts out as a Unicode string, so there should be no encoding issues, except insofar as the document encoding is used for post-load stuff like URIs.

I've since found the answer in section 4.2.5.5 of the whatwg spec. "If the document is an iframe srcdoc document, the document must not have a character encoding declaration. (In this case, the source is already decoded, since it is part of the document that contained the iframe.)"

Calling LoadStream isn't ok. It bypasses the sandboxing and Content Security Policy code in DoURILoad. Modifying DoURILoad to accept a srcdoc argument and create an InputStreamChannel instead of a normal one appears to work.

Improvements since the last patch: *DoURILoad now executes, so sandboxing and anything else present should still work (nothing has been conditionally disabled, so there may be some unnecessary code execution) *Can now set the URI to about:srcdoc without triggering security errors. To do this, about:srcdoc was registered as an about:page that resolves to an error page. Given the specifications state that it should be an unresolvable URI, is returning an error page acceptable? If not, the alternative would be to bypass the security checks when an about:srcdoc URI is accessed. Current work: I'm having difficulty setting the correct baseURI. As far as I can tell, setting the baseURI is an action performed on an nsIDocument, but I haven't been able to find where the iframe's document is created or located

Comment on attachment 684949 [details] [diff] [review] wip v2 Review of attachment 684949 [details] [diff] [review]: ----------------------------------------------------------------- Some nits: ::: content/base/src/nsFrameLoader.cpp @@ +328,3 @@ > > + mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::srcdoc, srcdoc); > + if (srcdoc.IsEmpty()) { This probably needs to check if the attribute is present, rather than if the value is empty. You can write that as if (!mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::srcdoc, srcdoc)) { ::: content/base/src/nsFrameLoader.h @@ +385,5 @@ > nsCOMPtr<nsIURI> mURIToLoad; > mozilla::dom::Element* mOwnerContent; // WEAK > > + nsString mSrcdoc; > + Please remove the trailing whitespace from this line ::: docshell/base/nsDocShell.cpp @@ +4498,5 @@ > > return InternalLoad(errorPageURI, nullptr, nullptr, > INTERNAL_LOAD_FLAGS_INHERIT_OWNER, nullptr, nullptr, > nullptr, nullptr, LOAD_ERROR_PAGE, > + nullptr, true, nullptr, nullptr,nullptr); Space after the last comma @@ +8701,5 @@ > // Do this asynchronously > nsCOMPtr<nsIRunnable> ev = > new InternalLoadEvent(this, aURI, aReferrer, aOwner, aFlags, > aTypeHint, aPostData, aHeadersData, > + aLoadType, aSHEntry, aFirstParty,aSrcdoc); Same here

Resolving to an error page will give incorrect behaviour for <iframe src="about:srcdoc"> I'll look into giving the security checks awareness of the srcdoc attribute and only bypassing the minimum required.

Improvements: This creates an exception within the security manager to permit a non resolvable about URI. I have tried to be specific as possible. There is an explicit opt-in flag and it will only work if the URI is "about:srcdoc". It now correctly handles empty srcdoc attributes. Left to do: Ensuring compliance with the standards. Properly setting origin, effective script origin, referrer, BaseURI etc (there is an attempt at the last one, but I doubt it works). I am having difficulty identifying where and how to make the necessary changes.

Comment on attachment 687384 [details] [diff] [review] wip v3 Boris, do you have any suggestions re: comment 16?

Comment on attachment 687384 [details] [diff] [review] wip v3 The right place to set the base URI is interesting. Per spec right now it sounds like this should return the base URL of the parent document, right? So what needs to happen is that documents need to know when they're srcdoc documents and nsIDocument::GetBaseURI needs to branch on that and do the right thing. Note that similar things need to happen for places where documents are used for referrer determination during fetch... It might be good to add a getter on nsIDocument that returns the parent document for srcdoc docs and the document itself otherwise or something. The right place to set the origin and effective script origin is by setting them on the channel when the load starts. There's existing code to do this for data: and javascript: and about:blank loads; we just need to do it in the srcdoc case too. I'm not sure what I think of the about:srcdoc setup in this patch. It might make more sense to just make it a normal about: URI that can be linked to for CheckLoadURI purposes but then make any asyncOpen attempts for it throw....

And sorry for the lack of response earlier. Please use the "need additional information from" checkbox thing if you have questions for me!

I'm having trouble working out how/where/when the child nsIDocument is created and how to reference it from within nsFrameLoader and nsDocShell. I've found code snippets within the files that return a nsCOMPtr<nsIDocument>, but I don't know if they return the desired one.

The child document is created when nsDSURIContentListener::DoContent calls CreateContentViewer. That ends up calling into the document loader factory, which lands us in nsContentDLF::CreateInstance which creates the actual document. Note that if the channel knows it's a srcdoc then the document's Reset() code can just get that information from the channel. In terms of getting a document in docshell, mContentViewer->GetDocument() if mContentViewer is not null. In frameloader, you there are several options, but the simplest is probably doing what nsFrameLoader::CreateStaticClone does, going via the docshell.

Progress: *Security manager hack has been removed and about:srcdoc is present for security manager purposes, yet unresolvable for AsyncOpen. *Referrer and base URI now follow specifications (albeit untested) *I think origin/effective script origin follows specifications, but I'm not convinced (again untested). Could you please confirm whether nsContentUtils::SetUpChannelOwner is responsible for setting the origin property? Remaining Tasks: * Tests. Where would be the most appropriate location for the tests to reside? * View source without a selection doesn't work. * Navigation is broken. Click on a link within the iframe, then click back and it tries to load the url about:srcdoc, rather than parse the srcdoc attribute.

Sorry for the lag here; I blame the weather and trying to get to Boston... I promise I'll comment on this no later than Monday.

SetUpChannelOwner sets up the .owner for the channel, for sure. The security context that's used then is the .owner if present, else derived from the channel's URI and whatnot. So the change to SetUpChannelOwner is a reasonable approach. The change to GetReferrer seems wrong to me. At this point the nsIDocument version is what's used from web script, so you want to be changing that one. Tests should probably go in content/base/test or content/html/content/test. I'd somewhat prefer the former. View source is broken presumably because it's using the URI... Need to think about this a bit. Similar for history navigation. I wonder whether using wyciwyg here like we do for document.open() is the way to go; conceptually srcdoc and document.open/write are somewhat similar... Olli, thoughts? That _might_ also make view-source work, by the way.

Comment on attachment 711757 [details] [diff] [review] wip v4 Having srcdoc-related members on docshell is a bit odd, since docshells live across multiple documents. I would much rather we stored that information on the channel or some other document-specific object. Same for the frameloader.

srcdoc is closer to data: urls than document.open(). Why would you use wyciwyg for srcdoc?

Because we can't stuff the data into the URI in this case: the URI is fixed by the spec. Well, I suppose we could stuff it in there but not serialize it when you call various getters on the URI. That would require a new URI class.

Testcase to assist with debugging for a non-empty origin (I've yet to integrate my debugger with the mochitest setup). It will be converted into a mochitest at some stage.

I'm splitting the patch into multiple sub-patches as it was starting to get a little unwieldy. The attached patch adds the ability to store srcdoc information in the channel. It currently stores the entire srcdoc string, but I'll change it to a bool if I determine the full information isn't necessary.

This one actually compiles.

adds srcdoc to the list of about pages It exists for security manager purposes yet is non-resolvable should anyone try to actually load it.

The change in mChannel's location will be reverted if storing the entire srcdoc string isn't necessary.

Now rewritten to embed information within an nsIChannel instance.

Improvements since wip v4. No longer stores srcdoc string. Now reports srcdoc support on html5test.com.

All patches have now been uploaded. Do you know of a way I can set a non-empty referrer for the parent document from within a mochitest? The current test compares two empty referrers and returns success, but that doesn't test for non-empty referrer propagation. Current work: There are still a few corner cases to address. I'm currently looking into one involving session history. Steps to reproduce: Create a srcdoc iframe with a link pointing to another site. Click on the link Click back. It should load the srcdoc page again. What it actually does is display an error page. I've identified that the session history entry (nsISHEntry) needs to be aware of srcdoc documents. The brute force approach is to embed the entire srcdoc string within the entry and updating session restore accordingly. Once people start using srcdoc iframes, this has the potential to add considerable bloat to session history. Can you think of a better way?

> Do you know of a way I can set a non-empty referrer for the parent document from within a > mochitest? Probably the simplest way is to load whatever document you want a nonempty referrer for in a subframe, right? Then the referrer should be the URI of the document the subframe is in. Embedding the scrdoc string in the session history entry is probably the way to go; it's no worse than what happens when people use a data: URI already... It's tempting to try to think of a way to encapsulate the srcdoc data inside the URI somehow, but I suspect that way ends up lying madness. So probably better to just store it explicitly.

Adds srcdoc support to the various nsIChannel implementations. This touches some very different places, so may need to be split up further for review. Also adds a new function to nsIViewSourceChannel to allow the embedded channel and URI to be replaced with an arbitrary channel. The purpose of this function is to allow the source of srcdoc iframes to be viewed. The alternative is to pass a srcdoc argument to NS_NewChannel and change all the protocol handlers to support an extra argument.

I don't think there were any changes with this patch, but uploading a new one to be safe. about:srcdoc exists in the listings, but returns an error if anyone tries to load it. The use of about:mozilla as the resolvable destination was chosen to make mistaken resolving obvious.

nsCOMPtr<nsIChannel> mChannel was moved from nsDocument.h to nsIDocument.h to avoid storing the srcdoc string in both the channel and the document.

Tests for compliance with specifications and that sandboxing works. Session history, session restore and view-source are not tested. Is this required and if so, how would you suggest doing it?

I've submitted this to try https://tbpl.mozilla.org/?tree=Try&rev=5322c0cb73c4 (The additional patch was for fixing mach mochitest-plain) The only issue is Win7-Opt not compiling properly, which is surprising given that I'm developing it on Windows. It doesn't give a clear error message and I can't see what went wrong. Can you suggest anything?

Forgot to modify the GUID.

@James: after you're done with this, you might be interested in taking a look at bug 631218. Nudge, nudge, hint, hint. ;)

I'm sorry for the review lag here. I'll be working on this tomorrow, for sure.

Comment on attachment 731635 [details] [diff] [review] Part 1: add srcdoc attribute to nsIChannel v2 Why put this on nsIChannel instead of, say, nsIInputStreamChannel? >+ void ReplaceChannel(in nsIURI aURI, Is this really ok to call at random points like halfway through a load? This is just needed for view-source on srcdoc documents, right? How do we plan to handle "view only this frame" or "view frame in new window" or whatnot with srcdoc?

> How do we plan to handle "view only this frame" or "view frame in new window" or whatnot > with srcdoc? In fact, those probably need to be raised as spec issues...

On the other hand, the spec author is likely to claim that it's not a spec issue or something, as long as the web-observable behavior is "right" (that being that the URI is an about:srcdoc). I'll post about it, but I'm not too hopeful. I need to think a little about how we want to handle those parts of the UI, in any case...

I posted http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2013-April/039333.html I guess what we implement here so far is no worse than what Safari does. :(

Comment on attachment 731636 [details] [diff] [review] Part 2: Add about:srcdoc to about protocol handlers Why do you need to change browser/components/about/AboutRedirector.cpp at all? Likewise for browser/components/build/nsModule.cpp I think you just want to add this to docshell/base/nsAboutRedirector.cpp and docshell/build/nsDocShellModule.cpp, plus your change to the about: protocol handler. And it should be fine to "redirect" srcdoc to "about:blank", not a chrome:// URI at all, I would think.

Comment on attachment 731639 [details] [diff] [review] Part 3: Changes to nsI?Document to support srcdoc >+ bool GetIsSrcdoc() { I'd prefer we call this "IsSrcdocDocument" and make it a const method. >+ void SetIsSrcdoc(bool aIsSrcdoc) { And here SetIsSrcdocDocuemnt. >+ nsresult GetSrcdoc(nsAString& aSrcdoc) { This could check mIsSrcDocDocument and bail early, right? Past that, it would need to QI to nsIInputStreamChannel and get the string from the channel.... Is there really a good reason to inline this? At first glance, I suspect not. We can just make it an nsIDocument method in nsDocument.cpp. >+ // Included within nsIDocument to avoid storing redundent srcdoc strings I don't follow why it's needed to avoid that. >+ bool mIsSrcdoc; msIsSrcdocDocument, and please put it next to the other boolean members a bit above this. >+ mIsSrcdoc(false) No need; documents are 0-initialized by their operator new. >+ nsAutoString srcdoc; >+ if (NS_SUCCEEDED(mChannel->GetSrcdoc(srcdoc))) { Hrm. I think we should just have a way to ask the channel for the boolean. r=me with the above fixed, but I'd like to see the updated patch.

Comment on attachment 731640 [details] [diff] [review] Part 4: add srcdoc support to docshell and SHEntry >+++ b/content/base/public/nsContentUtils.h >+ * from the channel. If aIsSrcdoc is true then the channel is for a srcdoc >+ * iframe and the principle is set as owner. That seems identical to aForceOwner in terms of behavior. Why can we not just pass aForceOwner == true? >+++ b/content/base/src/nsContentUtils.cpp >+ // XXX Should this be demoted to an assert? At best, or just removed if we reuse the existing aForceOwner. >+++ b/docshell/base/nsDocShell.cpp >+ nsXPIDLString srcdoc; nsString, please. >+ aLoadInfo->GetSrcdoc(getter_Copies(srcdoc)); I know other things here used "wstring" in the IDL, and I'm sorry we haven't updated them to not be late-90s code yet. :( Just use AString in the IDL, and then this will be aLoadInfo->GetSrcdoc(srcdoc); >@@ -1407,18 +1414,22 @@ nsDocShell::LoadURI(nsIURI * aURI, >+ return LoadHistoryEntry(shEntry, loadType, srcdoc.get(),flags); Please pass the srcdoc as an nsAString or nsString reference, not a PRUnichar pointer. And space before "flags". If the issue is that you need to differentiate whether there's a srcdoc at all or not, either pass it as an nsString* or use a void string to indicate lack of a srcdoc... >@@ -4667,43 +4682,57 @@ nsDocShell::Reload(uint32_t aReloadFlags >+ nsIDocument* cvDoc = mContentViewer->GetDocument(); Why can't you just use 'doc'? >@@ -8491,54 +8521,59 @@ void CopyFavicon(nsIURI *aOldURI, nsIURI >- >+ Please undo that change. >+ const PRUnichar *aSrcdoc) : Again, please pass around string objects, not PRUnichar pointers. >@@ -9431,41 +9471,102 @@ nsDocShell::DoURILoad(nsIURI * aURI, >+ nsAutoCString scheme; >+ rv = aURI->GetScheme(scheme); >+ NS_ENSURE_SUCCESS(rv,rv); >+ bool isViewSource = scheme.LowerCaseEqualsLiteral("view-source"); Please use SchemeIs() here. >+ if (aIsSrcdoc && isViewSource) { >+ // Hack. The input URI is about:srcdoc in this situation, but this >+ // cannot be passed to the view-source constructor as it is >+ // non-resolvable. Pass a dummy URI and correct for it further on >+ rv = NS_NewURI(getter_AddRefs(chanURI), >+ NS_LITERAL_STRING("view-source:about:blank")); I would almost rather we build knowledge of about:srcdoc into the view-source protocol handler so it won't bother to create the extra nested channel here. It could then also assert that the real channel is only being set on it once... Thoughts? >+ else { >+ chanURI = aURI; Please fix the indent. This file is 4-space indented, in general. >+ if (aIsSrcdoc) { >+ nsAutoCString cSrcdoc = NS_ConvertUTF16toUTF8(aSrcdoc); >+ rv = stream->SetData(cSrcdoc.get(), cSrcdoc.Length()); So this is ending up making at least 2 copies of the srdoc data in the common case.... I think it would be better to do something like this: uint32_t length; char* utf8Bytes = ToNewUTF8String(aSrcDoc, &length); rv = stream->AdoptData(utf8Bytes, length); >+ // XXX This could be replaced with a hardcoded about:srcdoc Yes, please! >+ rv = aURI->GetOriginCharset(charset); Don't bother. It doesn't really matter for this case. >+ vsChan->ReplaceChannel(aURI,srcdocChannel); Missing space after ','. >+ channel = do_QueryInterface(srcdocChannel); Why can't this just be a direct assignment? >+nsDocShell::LoadHistoryEntry(nsISHEntry * aEntry, uint32_t aLoadType, >+ if (aFlags & INTERNAL_LOAD_FLAGS_IS_SRCDOC) { Hmm. When would this be the case if aEntry->GetIsSrcdoc is false? >+ srcdoc = srcdocStr.get(); That sets srcdoc to a pointer to about-to-be-deallocated memory. Yet another reason to stick to string objects.... ;) >+ else { >+ // Do not inherit owner from document (security-critical!) >+ flags = INTERNAL_LOAD_FLAGS_NONE; And this branch never sets srcdoc at all, so it ends up with a random value. We clearly need more testing of this codepath. :( Or is that value unused if the INTERNAL_LOAD_FLAGS_IS_SRCDOC flag is not set? >+++ b/docshell/base/nsDocShellLoadInfo.cpp >+ NS_ENSURE_ARG_POINTER(aIsSrcdoc); Please don't bother null-checking XPCOM out params. And again, please make this AString in the IDL. >+++ b/docshell/base/nsIDocShell.idl > [noscript]void internalLoad(in nsIURI aURI, >+ in wstring aSrcdoc, Once we sort out the actual type here and how it interacts with the internal flag, we should document that. I think the type should be AString, which is ignored (or even asserted to be empty) if the srcdoc load flag is not set. >+++ b/docshell/base/nsIDocShellLoadInfo.idl >+ attribute wstring srcdoc; AString This is looking really good; most of the stuff above is little mechanical nits, but the overall approach seems totally reasonable. When you post an updated version for this one, I'd really appreciate an interdiff; let me know if you're not sure how to produce one.

Comment on attachment 731641 [details] [diff] [review] Part 5: Allow sessionrestore to work on srcdoc r=me

Comment on attachment 731641 [details] [diff] [review] Part 5: Allow sessionrestore to work on srcdoc Would like a once-over from a sessionstore peer here.

Comment on attachment 731840 [details] [diff] [review] Part 6: HTML related changes to about srcdoc v2 > nsFrameLoader::LoadFrame() >+ bool isSrcdoc = mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::srcdoc, srcdoc); You're not using the value here, right? If so, better to do: bool isSrcdoc = mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::srcdoc); >@@ -470,18 +478,35 @@ nsFrameLoader::ReallyStartLoadingInterna >+ rv = NS_NewURI(getter_AddRefs(referrer), referrerStr, charset); I wouldn't worry about the charset here, honestly. Especially because the document's charset may have not much bearing on the referrer string's bytes, since that string came from the _previous_ document. r=me on the non-parser bits. Henri, would you look over the parser changes?

Comment on attachment 731643 [details] [diff] [review] Part 7: Testcases of srcdoc iframes >* * * >try: -b o -p all -u all -t none Drop this part, please. ;) Would be good to have session history tests too, but those are a huge pain, so maybe just manual testing is OK for now. Of course if you're up for it... ;) >+++ b/content/base/test/test_srcdoc.html This should maybe live under content/html/content/test >+ var iframe = pframe.getElementById("iframe"); Shouldn't that be pframe.contentDocument.getElementById("iframe")? Not sure how this is passing... >+ var innerDoc = iframe.contentDocument || iframe.contentWindow.document; We support contentDocument, so no need for the second half. ;) All three times, please. r=me with those nits. And again, this is looking really good. Thank you for sticking with it!

Comment on attachment 731635 [details] [diff] [review] Part 1: add srcdoc attribute to nsIChannel v2 I think we should just put this on input stream channels and QI where we care.

Ah, you asked about tests for the edge cases. So for viewsource the only thing I can think of is a browser-chrome test or something. I have no idea how to write one for that end of it offhand; would have to ask around on irc... I think it's ok to pass on that for now. Session restore is even worse. Again, asking on irc is the best bit if you want to write a test for it. For session history, you should be able to just write a test that loads an iframe A that has a srcdoc iframe B inside, and then does the following: 1) Add an unload listener to the window inside iframe A (to disable bfcache for it). 2) Modify the DOM of the document inside iframe A in some way (e.g. add an element with some id) 3) Starts an about:blank load in iframe A. 4) When that load finishes, call history.back(). 5) When _that_ load finishes, verify that the modification made in step 2 is gone, so we didn't load from bfcache. 6) Check that the srcdoc bit got loaded properly. I think that should do the trick... > The only issue is Win7-Opt not compiling properly That looks like infrastructure bustage to me, not your code.

Comment on attachment 731641 [details] [diff] [review] Part 5: Allow sessionrestore to work on srcdoc forwarding to speed it up.

Comment on attachment 731840 [details] [diff] [review] Part 6: HTML related changes to about srcdoc v2 r- on the parser changes. They appear to be incomplete. The translator does not run as part of the build, so there are C++ bits missing. Even the Java version of the AttributeName needs to regenerate its hashes for the Java version to make sense and before it can be translated to C++. The good news is that the parser changes are unnecessary to land this. Making the parser aware of srcdoc is a mere performance optimization that can be landed separately. (I can do it.) r+ if the changes under parser/html/ are simply omitted from this patch (or, alternatively, made complete, but as noted above, you don't need to do that).

What part of these patches makes sure that the doc parsed out of srcdoc reports its encoding the right way? Upon a quick look, the spec isn't clear on what the right way is. I hope it is that the doc always reports UTF-8 as its encoding. If the spec doesn't already say so, is there a spec bug to that effect on file?

> What part of these patches makes sure that the doc parsed out of srcdoc reports its > encoding the right way? The relevant bit is: + NS_NewInputStreamChannel(getter_AddRefs(srcdocChannel), inStreamURI, stream, + NS_LITERAL_CSTRING("text/html"), + NS_LITERAL_CSTRING("UTF-8")); which will make us report "UTF-8". In terms of the spec, I have no clue what it's requiring here, if anything; scrdoc is monkeypatched in all over in no particularly principled way, so I can't tell what the right behavior is for it.

James, looking at the spec there are three more issues that need to be dealt with as far as I can tell: 1) Changes to @srcdoc should do the "process frame attributes" thing. 2) Changes to @src should be ignored if @srcdoc is set. 3) The srcdoc document should always be standards mode. We should add tests for all three of those...

I've addressed almost all of the non-optional comments and will submit the patches for review once the final matter is resolved. (In reply to Boris Zbarsky (:bz) from comment #48) > This is just needed for view-source on srcdoc documents, right? How do we > plan to handle "view only this frame" or "view frame in new window" or > whatnot with srcdoc? I have the contents of the srcdoc attribute displaying in these cases in standards compliance mode. What I have doubts about is what to do with domain/origin and referrer. For a srcdoc iframe, both of these are determined based on the "Document's browsing context's browsing context container's Document". The question is: Does the contents of a srcdoc iframe, when viewed in isolation in a new tab/window, have a "browsing context container"? If it doesn't, origin/referrer are undefined, and current behaviour (blank or null) should be adequate. If it does have a browsing context container which inherits some behaviour/attributes from the parent document, the nature of this needs to be carefully specified as there is the potential for some rather perverse outcomes. I've looked through the relevant posts on the whatwg mailing list, but was unable to come to a conclusion.

One further complication has crossed my mind. When a srcdoc iframe is viewed in isolation, the address bar is currently set to about:srcdoc (as that is the URL of the document according to the specifications). Up until now, all about: documents are internal to Firefox and resolve to chrome: URIs. I'm a little concerned about the potential for users to mistake an about:srcdoc document for an internal firefox page, rather than the arbitrary web content that it is.

> Does the contents of a srcdoc iframe, when viewed in isolation in a new tab/window, have > a "browsing context container"? Nope. > Up until now, all about: documents are internal to Firefox and resolve to chrome: URIs. Not necessarily; about:blank doesn't, and neither do some of the ones extensions add... But yes, that's a bit of a worry. The other option is to just disable those context menu options for srcdoc iframes, like roc suggested. Gavin?

(In reply to Boris Zbarsky (:bz) from comment #68) > But yes, that's a bit of a worry. The other option is to just disable those > context menu options for srcdoc iframes, like roc suggested. This sounds reasonable. It sounds like we probably want all of the "is this a frame?" checks in nsContextMenu to return false for srcdoc iframes.

Comments have been applied. isSrcdocChannel/srcdocData are now nsIInputStreamChannel attributes. One side effect is that viewSourceChannels no longer have these attributes, which limits the ability to recreate the channel at a later date. However I am unaware of a situation in which this might be necessary. There is a new approach for viewSourceChannels. Rather than creating loading view-source:about:blank and overwriting it with the srcdoc channel, the viewSourceHandler has been given an additional constructor which accepts a srcdoc string. It still embeds a nsIViewSourceChannel, but creation of the channel is now done within nsIViewSourceChannel.

Comments have been applied

Comments have been applied. Flagging for review again as you asked to see it again and the changes are non-trivial. isSrcdocDocument has been exposed to chrome-only javascript to allow the context menu to detect srcdoc iframes. Behaviour of GetSrcdocData has changed. It always returns NS_OK and lack of data is indicated by a NullString outparam.

Interdiff of the docshell changes. I doubt it will apply cleanly to the previously submitted patch as there was a need to rebase it. Changes were made to SHEntry to make isSrcdoc/srcdoc more explicit (now isSrcdocEntry/srcdocData)

This is the docshell changes folded into the original rebased patch. The differences between this patch and the original are found in attachment 742791 [details] [diff] [review].

Superficial changes to work with the changes to SHEntry. isSrcdoc -> isSrcdocEntry srcdoc -> srcdocData

Changes have been applied. changes to /parser have been removed.

Following the suggestion in comment 69, this prevents srcdoc iframes from being detected as frames within the context menu.

Test for session history has been added. Tests for other matters have not.

Comment on attachment 742795 [details] [diff] [review] Part 6: HTML related changes to about srcdoc v3 On second thoughts, this might require review for the changes to standards mode and loading on src/srcdoc changes.

(In reply to James Kitchener from comment #78) > Created attachment 742798 [details] [diff] [review] > Part 7: Testcases of srcdoc iframes v2 > > Test for session history has been added. > > Tests for other matters have not. Tests for standards compliance and loading on src/srcdoc changes have also been added.

Comment on attachment 742786 [details] [diff] [review] Part 1: add srcdoc attribute to nsIChannel v3 I think we should make isScrdocChannel readonly and just have it return whether scrdocData was ever set. >+ // about:srcdoc as this is the only permissable URL for srcdoc "permissible". >+ rv = NS_NewURI(getter_AddRefs(inStreamURI), >+ NS_LITERAL_STRING("about:srcdoc")); Please fix the indent. >+ NS_ENSURE_SUCCESS(rv,rv); Space after comma. This comes up twice in this patch. >+ NS_NewInputStreamChannel(getter_AddRefs(mChannel), inStreamURI, stream, You presumably meant to assign the return value of this into rv, right? >+ if (inStrmChan) { It can't be null. Feel free to assert that! >+ inStrmChan->SetSrcdocData(nsAutoString(aSrcdoc)); Just inStrmChan->SetSrcdocData(aSrcdoc), please. r=me with those nits. Benjamin, would you mind doing an sr on the IDL change here?

Comment on attachment 742788 [details] [diff] [review] Part 2: Add about:srcdoc to about protocol handlers r=me

Comment on attachment 742790 [details] [diff] [review] Part 3: Changes to nsI?Document to support srcdoc v2 >+++ b/content/base/public/nsIDocument.h >+ nsCOMPtr<nsIChannel> mChannel; You need to remove that from nsDocument.h, right? Sorry I misseed that last time. :( >+++ b/content/base/src/nsDocument.cpp >+ nsAutoString srcdocData; This variable is unused. Please remove it. >+nsDocument::GetIsSrcdocDocument(bool* aIsSrcdocDocument) Hmm. Is this used anywhere? If not, I would prefer that we just not add this to nsIDOMDocument.idl and not add this XPCOM method. >+nsIDocument::GetSrcdocData(nsAString &aSrcdocData) >+ if(mIsSrcdocDocument && mChannel) { Space after "if", please. Also, no need to null-check mChannel here, since do_QueryInterface is null-safe. >+ inStrmChan->GetSrcdocData(aSrcdocData); >+ return NS_OK; Probably better to do: return inStrmChan->GetSrcdocData(aSrcdocData); >+++ b/dom/interfaces/core/nsIDOMDocument.idl Like I said, probably better to just drop the changes to this file. r=me with the above nits.

Though I guess part 7 may be relying on the nsIDOMDocument bits for now because HTMLDocument is not on WebIDL bindings yet... If so, it's OK to land that with a followup bug to remove it as soon as bug 855971 is fixed.

Comment on attachment 742795 [details] [diff] [review] Part 6: HTML related changes to about srcdoc v3 >+++ b/content/base/src/nsFrameLoader.cpp >+ // Drop that line, please. >+ nsCOMPtr<nsIDocument> doc; No need for that temporary. Just do: mOwnerContent->OwnerDoc()->GetReferrer(referrerStr); >+++ b/content/html/content/src/nsGenericHTMLFrameElement.cpp >@@ -219,18 +219,24 @@ nsGenericHTMLFrameElement::SetAttr(int32 >+ //Changes to src have no effect if srcdoc attribute is present Space after "//", please. You need to change UnsetAttr too, I think. Specifically, unsetting @srcdoc should LoadSrc(), I would think. Also, you want to have the srcdoc stuff here only affect <iframe>, not <frame>. That applies for the nsFrameLoader bits too, I just realized: those should only do srcdoc stuff when mOwnerContent->IsHTML(nsGkAtoms::iframe). >+++ b/content/html/document/src/nsHTMLDocument.cpp > nsHTMLDocument::SetCompatibilityMode(nsCompatibility aMode) Does this affect quirks mode stuff in the parser itself? For example does this: <iframe srcdoc='<p style="border: 1px solid black"><table></table>Text</p>'></iframe> put the text in the black box or after it? It should go after the black box... Furthermore, I just looked at the spec more carefully, and it looks like standards mode is only the default if no doctype token was seen. It's possible that this was a spec change since the last time I reviewed. :( Henri, what's the right way of fixing the parser here?

Comment on attachment 742798 [details] [diff] [review] Part 7: Testcases of srcdoc iframes v2 >+ function soon(f) { Use SimpleTest.executeSoon. >+ pframe.addEventListener("unload", function () { }, false); So this assumes pframe is the window in the iframe... >+ pframe.onload = soon(function () { But this assumes it's the iframe itself. The latter makes more sense to me; should the addEventListener be happening on pframe.contentWindow? >+++ b/content/html/content/test/test_srcdoc.html This test seems to assume that @scrdoc and @src changes take effect synchronously. That's not right... It should probably also be testing removeAttribute("srcdoc"). about:robots is a Firefox-specific about: URI, but this is a Gecko test. Use some data: URI instead, please.

Comment on attachment 742793 [details] [diff] [review] Part 4: add srcdoc support to docshell and SHEntry v2 >+ if(isViewSource) { Space after "if", please. >+ nsViewSourceHandler *vsh = new nsViewSourceHandler(); Protocol handlers are services; you don't want to create a new one. What's probably better is to have a private |static nsViewSourceHandler* gInstance| in the nsViewSourceHandler class with a public static getter for it, set it when an instance is created, null it out when the instance is destroyed, and use it here. >+ inStrmChan->SetSrcdocData(nsAutoString(aSrcdoc)); Same comments here as before. Can we we share this chunk of code (e.g. by putting it in nsNetUtil.h or something)? >@@ -10557,16 +10621,30 @@ nsDocShell::AddToSessionHistory(nsIURI * >+ if (aChannel) { Again, no need to null-check the argument to do_QueryInterface; just null-checking the QI result is enough. >@@ -10703,28 +10781,43 @@ nsDocShell::LoadHistoryEntry(nsISHEntry >+ else { >+ srcdoc = NullString(); Again, 4-space indent. >+ NullString(), // XXX No srcdoc? Take out the "XXX" and the question mark; there is no srcdoc for a link click, of course. ;) >+++ b/docshell/base/nsDocShellLoadInfo.cpp >+NS_IMETHODIMP nsDocShellLoadInfo::SetIsSrcdocLoad(bool aIsSrcdocLoad) Can we just automatically set this boolean to true when SetScrdocData is called, and get rid of this setter? r=me with those nits fixed, though I'd like to take a look at the new view source handler code. Thank you very much for the interdiff!

(In reply to Boris Zbarsky (:bz) from comment #83) > Comment on attachment 742790 [details] [diff] [review] > Part 3: Changes to nsI?Document to support srcdoc v2 > > >+++ b/content/base/public/nsIDocument.h > >+ nsCOMPtr<nsIChannel> mChannel; > > You need to remove that from nsDocument.h, right? Sorry I misseed that last > time. :( > Oops. Thought I fixed that. > > >+nsDocument::GetIsSrcdocDocument(bool* aIsSrcdocDocument) > > Hmm. Is this used anywhere? If not, I would prefer that we just not add > this to nsIDOMDocument.idl and not add this XPCOM method. > > >+++ b/dom/interfaces/core/nsIDOMDocument.idl > > Like I said, probably better to just drop the changes to this file. > > r=me with the above nits. This function is used to give the context menu awareness of srcdoc iframes (see attachment 742796 [details] [diff] [review]: Part 7: srcdoc iframes no longer considered frames in context menu). For reasons I don't understand "document.isSrcdocDocument" calls nsIDocument:IsSrcdocDocument() while "this.target.ownerDocument.isSrcdocDocument" will only call nsDocument::GetIsSrcdocDocument() Is there a better way to detect srcdoc iframes within chrome javascript? (srcdoc status is otherwise stored within the parent document's iframe element, but I don't know of a way to access that from the child document object).

> For reasons I don't understand It's because HTMLDocument is using WebIDL quickstubs but not actual WebIDL bindings yet, so access via an XrayWrapper goes through the XPConnect interface so far. See comment 84. The other option would be to use something like doc.defaultView.frameElement in the chrome JS to get at the actual <iframe> and then check whether the "srcdoc" attribute is set, I guess. It would be slightly racy when the attribute gets changed but the new document hasn't loaded yet, and I don't think it's worth worrying about, per comment 84. In fact, thinking about it more, once bug 855971 lands the main reason I don't like the nsIDOMDocument change (that it exposes the method unconditionally to web content) goes away, so we should just add this to nsIDOMDocument and move on.

(In reply to Boris Zbarsky (:bz) from comment #85) > >+++ b/content/html/document/src/nsHTMLDocument.cpp > > nsHTMLDocument::SetCompatibilityMode(nsCompatibility aMode) > > Does this affect quirks mode stuff in the parser itself? For example does > this: No it doesn't. The document still renders in quirks mode. > > <iframe srcdoc='<p style="border: 1px solid > black"><table></table>Text</p>'></iframe> > > put the text in the black box or after it? It should go after the black > box... > > Furthermore, I just looked at the spec more carefully, and it looks like > standards mode is only the default if no doctype token was seen. It's > possible that this was a spec change since the last time I reviewed. :( > > Henri, what's the right way of fixing the parser here? So far I've identified the portion of code responsible for setting the compatibility mode in the absence of a doctype within the parser, but I haven't been able to find a way to make it srcdoc aware (a property held in nsIInputStreamChannel or nsI?Document instances).

Henri, what's a good way to handle the quirks mode bits here?

Comment on attachment 742795 [details] [diff] [review] Part 6: HTML related changes to about srcdoc v3 > Furthermore, I just looked at the spec more carefully, and it looks like > standards mode is only the default if no doctype token was seen. Eww. > Henri, what's the right way of fixing the parser here? To introduce an srcdoc flag on the TreeBuilder and making the doctype handling logic branch on the flag in the case of a missing doctype. Aside: I see that the spec says: "For iframe elements in XML documents, the srcdoc attribute, if present, must have a value that matches the production labeled document in the XML specification." That's not cool. It seems to repeat the same mistake that was made with innerHTML when making the parser it invokes depend on the HTMLness flag of the document.

(In reply to Henri Sivonen (:hsivonen) from comment #92) > To introduce an srcdoc flag on the TreeBuilder and making the doctype > handling logic branch on the flag in the case of a missing doctype. Making the TreeBuilder aware of whether it's involved in parsing the contents of an srcdoc attribute that is.

Then again: What's the use case for supporting non-standards modes for srcdoc? Why don't we ask for a spec chance to make the mode always be the standards mode for srcdoc?

> when making the parser it invokes depend on the HTMLness flag of the document. Actually, in this case things are fine, from our pov. The "must have a value that matches the production" stuff is an _authoring_ requirement (though why it's there, I dunno). The UA requirement is to parse it as text/html. > Why don't we ask for a spec chance to make the mode always be the standards mode for > srcdoc? I thought it used to say that and got changed to the present behavior. But looks like it's just been this way all along. Let's do that and I filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=21998

Comment on attachment 742796 [details] [diff] [review] Part 7: srcdoc iframes no longer considered frames in context menu Sorry for the delay here, I lost track of this request. One real issue: this change will hide the "view image"/"view video" context menu items for images/videos in srcdoc frames, which probably isn't optimal. To fix that we'd need to track inFrame and inSrcdocFrame separately, I think. Can you file a followup bug for that? One nit: please put a before the parenthesis in "if("

I've attached an attempt at giving the parser awareness of srcdoc iframes, generated as appropriate from the Java source. It fails to work approximately half the time when run and sometimes causes non-srcdoc documents without doctypes to render in standards mode. (With a debugger the iframe is always correctly rendered and the parent document is always incorrectly rendered) I've identified that nsHtml5Parser correctly detects srcdoc documents, but the isSrcdocDocument flag does not report the correct value by the time it is actually read. Is there a more appropriate location to set the isSrcdocDocument flag and a more reliable way to reset it so it doesn't affect other documents? (Note that this is a subset of the changes. I haven't included changes to the generated NamedCharacters and AttributeName files since they increase the patch size by roughly 400kB. I also haven't included changes to the copyright date or ones that replace int32_t with PRInt32 which were caused by regenerating the cpp files rather than my changes.)

(In reply to James Kitchener from comment #97) > I've attached an attempt at giving the parser awareness of srcdoc iframes, > generated as appropriate from the Java source. Looks good, though I'd put "else" on the same line with "}". Also, it looks like it fails to suppress some errors in the case the token that triggers the mode commitment is a character token. > It fails to work approximately half the time when run and sometimes causes > non-srcdoc documents without doctypes to render in standards mode. (With a > debugger the iframe is always correctly rendered and the parent document is > always incorrectly rendered) This is probably due to nsHtml5Parser and nsHtml5StreamParser having distinct instances of nsHtml5TreeBuilder. Instead of debugging this further right now, I suggest pursuing https://www.w3.org/Bugs/Public/show_bug.cgi?id=21998 > ones that replace int32_t with PRInt32 which were caused > by regenerating the cpp files rather than my changes.) Interesting. I will need to follow up on that one. I thought I had landed a fix.

A NS_NewInputStreamChannel overload has been added which takes an nsAString as input and uses it to create the InputStream.

The nsIDOMDocument changes have been removed as they are no longer needed.

An additional change has been made to SetUpChannelOwner to disable an assert for about:srcdoc URIs. This can cause false negatives for non-srcdoc iframes whenever the URL is set to about:srcdoc, but it shouldn't matter as the URI is non-resolvable.

Corrections have been made.

Now renders all srcdoc iframes in standards mode. Most doctype error messages are suppressed from within the error function itself. (The errStrayDoctype error isn't suppressed as the standards don't allow arbitrary doctype locations.)

Tests should now be asynchronous. For one of the edge cases, it was necessary to test than an event didn't occur. The time period before assuming success is currently 2 seconds, but that is a guess at a reasonable interval.

Comment on attachment 764135 [details] [diff] [review] Part 1: add srcdoc attribute to nsIChannel v4 I assume you meant to ask for review... What compile errors did you hit with the readable utils version of ToNewUTF8String? >+ aSrcdoc,NS_LITERAL_CSTRING("text/html"), Space after comma.

Comment on attachment 764138 [details] [diff] [review] Part 3: Changes to nsI?Document to support srcdoc v3 The documentation for GetSrcDocData (about NS_ERROR_NOT_AVAILABLE) does not match the implementation....

The old part 4 seems to have become part 5. What's part 4 now?

Comment on attachment 764139 [details] [diff] [review] Part 4: add srcdoc support to docshell and SHEntry v3 We still have the SetIsSrcdocLoad and SetIsSrcdocEntry setters that seem like they could be handled by SetSrcdocData directly...

Comment on attachment 764141 [details] [diff] [review] Part 6: HTML related changes to about srcdoc v4 The frameloader parts seem to be mis-merged (e.g. end up doing GetURL twice). >+ // Fall back to the src element, if any "src attribute". This is still doing scrdoc stuff for <html:frame>. Was there a reason for that, or just oversight? r- pending this being explained. Note that the spec has been updated to say that srcdoc is always standards-mode, for what that's worth...

Comment on attachment 764153 [details] [diff] [review] Part 9: Testcases of srcdoc iframes v3 >+ ok(!iframe1load, "Changing scr attribute shouldn't cause a load when srcdoc is set"); s/scr/src/ r=me

Comment on attachment 764139 [details] [diff] [review] Part 4: add srcdoc support to docshell and SHEntry v3 Oops. Part 4 (docshell) should remain part 4. The old part 5 (session restore) is still part 5.

(In reply to Boris Zbarsky (:bz) from comment #107) > What compile errors did you hit with the readable utils version of > ToNewUTF8String? When compiling intl/unicharutil/tests/UnicharSelfTest.cpp and intl/unicharutil/tests/NormalizationTest.cpp: 86:32.93 c:\mozilla-central\obj-i686-pc-mingw32\dist\include\nsStringFwd.h(16) : fatal error C1004: unexpected end-of-file found Which is a MSVC bug caused when the #error at line 16 is triggered. #error Internal string headers are not available from external-linkage code.

OK. Can you just ifdef MOZILLA_INTERNAL_API that function in nsNetUti.h to get rid of that problem with the tests?

Code paths for MOZILLA_INTERNAL_API and otherwise are now included.

Comment has been corrected.

isSrcdoc(Load|Entry) now readonly.

Fallout from making isSrcdocEntry readonly.

It was an oversight. I believe this patch acts upon the correct file.

Rebased, but otherwise no further changes.

typo fixed

Comment on attachment 765306 [details] [diff] [review] Part 6: HTML related changes Instead of duplicating the SetAttr() code, I think we should do the following: 1) HTMLIFrameElement::SetAttr only handles changes to the srcdoc attribute and calls into nsGenericHTMLFrameElement::SetAttr. 2) nsGenericHTMLFrameElement::SetAttr only responds to @src changes if Tag() != nsGkAtoms::iframe || !HasAttr(kNameSpaceID_None, nsGkAtoms::srcdoc). You also need IsHTML(nsGkAtoms::iframe) checks in the frameloader bits that look for the srcdoc attr. r=me with the above changes.

HTMLIFrameElement::SetAttr should have a two-space indent. Also, it may be clearer in frameloader to do: bool isSrcdoc = mOwnerContent->IsHTML(nsGkAtoms::iframe) && mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::srcdoc); and nsAutoString srcdoc; bool isSrcdoc = mOwnerContent->IsHTML(nsGkAtoms::iframe) && mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::srcdoc, srcdoc);

rebased

One word fix to allow it to compile on other platforms.

Slight reordering to allow it to compile on other platforms.

and another rebase. Green on try https://tbpl.mozilla.org/?tree=Try&rev=519607c96b3e Unless there are any further issues, this may be ready to land.

Looks good to me. Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/49375972f4b4 https://hg.mozilla.org/integration/mozilla-inbound/rev/6c5c581892e7 https://hg.mozilla.org/integration/mozilla-inbound/rev/da373b72ad88 https://hg.mozilla.org/integration/mozilla-inbound/rev/3758ba469a3a https://hg.mozilla.org/integration/mozilla-inbound/rev/3330a8c136b6 https://hg.mozilla.org/integration/mozilla-inbound/rev/6e0e2baaede1 https://hg.mozilla.org/integration/mozilla-inbound/rev/9b6ae7ea5802 https://hg.mozilla.org/integration/mozilla-inbound/rev/693d37696683 https://hg.mozilla.org/integration/mozilla-inbound/rev/f3c22357f4f7 James, thank you for sticking with this!

https://hg.mozilla.org/mozilla-central/rev/49375972f4b4 https://hg.mozilla.org/mozilla-central/rev/6c5c581892e7 https://hg.mozilla.org/mozilla-central/rev/da373b72ad88 https://hg.mozilla.org/mozilla-central/rev/3758ba469a3a https://hg.mozilla.org/mozilla-central/rev/3330a8c136b6 https://hg.mozilla.org/mozilla-central/rev/6e0e2baaede1 https://hg.mozilla.org/mozilla-central/rev/9b6ae7ea5802 https://hg.mozilla.org/mozilla-central/rev/693d37696683 https://hg.mozilla.org/mozilla-central/rev/f3c22357f4f7

The documentation has been updated: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe#attr-srcdoc https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement and https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/25

Add under the HTML5 section.

James Kitchener from comment #130) > Created attachment 769298 [details] [diff] [review] > Part 9: tests Noticed that this feature is covered by automated tests. Is anything in particular you want covered by QA? If yes, please let me know how could I help.

There are a few items that aren't covered by automated tests. 1. "View frame source" for a srcdoc iframe. (The presence of the menu option is checked, but whether it works is not. 2.Session restore. Navigate to a page with a srcdoc iframe, perhaps navigate the iframe to another page, close and then reopen the browser. The iframe should then correctly display the srcdoc page (in the navigation history if the frame loaded another page). I did check that they worked locally, but there may be corner cases I hadn't considered or things have broken since I last tested them.

Adding the feature keyword so that this bug is properly picked up by the Release Tracking wiki page.