Closed Bug 964239 Opened 6 years ago Closed 6 years ago

view-source fails in srcdoc iframes with relative links

Categories

(Core :: HTML: Parser, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jkitch, Assigned: jkitch)

References

Details

Attachments

(7 files, 2 obsolete files)

Attached file viewframe.html
View the frame source of the "bad" iframe of the attached testcase.  The resulting source cuts off at "<a href=", even if there is more content placed after the link.  As the two "good" cases show, this appears to only affect relative URI's.

I've identified the failure as being caused by this line:
https://hg.mozilla.org/mozilla-central/annotate/611698b4a246/parser/html/nsHtml5TreeOperation.cpp#l756

aBuilder->GetViewSourceBaseURI() in this instance returns about:srcdoc, which is passed into NS_NewURI as the base URI.  NS_NewURI believes this to be invalid and returns NS_ERROR_MALFORMED_URI, which causes the rest of the parsing to fail.

The base URI of an about:srcdoc iframe is the base URI of the page which contains the iframe, so GetViewSourceBaseURI should return this instead.
This renders invalid URIs as strings rather than giving up completely and ignoring the rest of the document.

The same practice is performed for unsafe (javascript:) URLs, so it shouldn't be any worse for arbitrary rubbish in the href field.
Assignee: nobody → jkitch.bug
Status: NEW → ASSIGNED
Attachment #8367805 - Flags: review?(hsivonen)
Attachment #8367805 - Flags: review?(hsivonen) → review+
Attachment #8367805 - Attachment description: fix.diff → Part 1: Fallback URL parsing
Attached patch Part 2: Protocol changes (obsolete) — Splinter Review
The problem with getting relative links to work with view-source:about:srcdoc, is that I am aware of no way to recover the base URI at that time.  The document's parent baseURI, although used for srcdoc document purposes, isn't applicable as it returns a chrome: URI.  ownerPrincipal will get the domain right, but may return the wrong subfolder.

To solve this, the base URI is embedded in the appropriate channels (nsIInputStreamChannel and nsIViewSourceChannel).
Attachment #8368411 - Flags: review?(bzbarsky)
Attached patch Part 3: docshell changes (obsolete) — Splinter Review
Mostly mechanical changes to support passing a new argument to represent baseURI within docshell and session history.

Given that it is a nullptr except where explicitly set, should baseURI instead be called baseURIOverride?  srcdocBaseURI may not be appropriate if these changes can be generalised to fix bug 464222.
Attachment #8368413 - Flags: review?(bzbarsky)
The base URI is determined by the frame's parent's base URI.
Attachment #8368416 - Flags: review?(bzbarsky)
The new session history attribute is included in session restore to allow viewing the source of a srcdoc iframe which was loaded via session restore.

Although currently only used if isSrcdocEntry is true, its use may be expanded in future patches.
Attachment #8368419 - Flags: review?(ttaubert)
When determining the base URI, this checks the channel to see if an override value has been set when it cannot otherwise be inferred.

If the override base URI is not present or the steps to obtain it fail, the code falls back to the old method.

These changes coexist with Part 1.  Part 1 allows parsing the webpage to continue if no sensible base URL is found.  Parts 2-6 allow a sensible value to be found in more circumstances.
Attachment #8368421 - Flags: review?(hsivonen)
Duplicate of this bug: 915025
This has been tested locally, but there are no automated tests.

I'm open to suggestions on how to create them.
Attachment #8368421 - Flags: review?(hsivonen) → review+
Attachment #8368419 - Flags: review?(ttaubert) → review+
Comment on attachment 8368411 [details] [diff] [review]
Part 2: Protocol changes

Sorry for the lag here..

>+++ b/netwerk/protocol/viewsource/nsViewSourceChannel.cpp
>+    if (isc) {

Given how we create it, this better not be null.  I guess it doesn't really hurt to check, but could also just assert.

>+nsViewSourceChannel::GetBaseURI(nsIURI** aBaseURI)

Again, seems like in the mIsSrcdocChannel case we know mChannel is an input stream channel.

r=me
Attachment #8368411 - Flags: review?(bzbarsky) → review+
Comment on attachment 8368413 [details] [diff] [review]
Part 3: docshell changes

Again, not sure we need the null-check after QI if we just did NS_NewInputStreamChannel

>+                             nullptr,                   // Not needed

Maybe "// baseURI not needed"?

r=me
Attachment #8368413 - Flags: review?(bzbarsky) → review+
Comment on attachment 8368416 [details] [diff] [review]
Part 4: content changes

r=me
Attachment #8368416 - Flags: review?(bzbarsky) → review+
Attachment #8368413 - Attachment is obsolete: true
(In reply to Boris Zbarsky [:bz] from comment #9)
> Comment on attachment 8368411 [details] [diff] [review]
> Again, seems like in the mIsSrcdocChannel case we know mChannel is an input
> stream channel.

This has been left unchanged as I don't think it is guaranteed.  A different mChannel can be assigned in nsViewSourceChannel::OnStartRequest(), which might not be an nsIInputStreamChannel.
Attachment #8368411 - Attachment is obsolete: true
Keywords: checkin-needed
The problem is that I haven't worked out how to test it.

Viewing the source of an about:srcdoc document only works when it resides in session history (it can't rely on URL alone), and I haven't found a way to write an automated test that can view the source of something in session history.

I have worked out how to test the follow-up bug (bug 464222), which will exercise the parser, ViewSourceChannel and most of the docshell changes from this bug.
You need to log in before you can comment on or make changes to this bug.