Closed
Bug 964239
Opened 10 years ago
Closed 10 years ago
view-source fails in srcdoc iframes with relative links
Categories
(Core :: DOM: HTML Parser, defect)
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: jkitch, Assigned: jkitch)
References
Details
Attachments
(7 files, 2 obsolete files)
196 bytes,
text/html
|
Details | |
1.21 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
954 bytes,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
1.66 KB,
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
1.68 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
26.79 KB,
patch
|
Details | Diff | Splinter Review | |
9.54 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8367805 -
Flags: review?(hsivonen) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8367805 -
Attachment description: fix.diff → Part 1: Fallback URL parsing
Assignee | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
The base URI is determined by the frame's parent's base URI.
Attachment #8368416 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
This has been tested locally, but there are no automated tests. I'm open to suggestions on how to create them.
Updated•10 years ago
|
Attachment #8368421 -
Flags: review?(hsivonen) → review+
Updated•10 years ago
|
Attachment #8368419 -
Flags: review?(ttaubert) → review+
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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 11•10 years ago
|
||
Comment on attachment 8368416 [details] [diff] [review] Part 4: content changes r=me
Attachment #8368416 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8368413 -
Attachment is obsolete: true
Assignee | ||
Comment 13•10 years ago
|
||
(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
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/028dbae2c819 https://hg.mozilla.org/integration/mozilla-inbound/rev/710e2f5dca1c https://hg.mozilla.org/integration/mozilla-inbound/rev/bdf66d134585 https://hg.mozilla.org/integration/mozilla-inbound/rev/c0e3b8b4e441 https://hg.mozilla.org/integration/mozilla-inbound/rev/b16e90588530 https://hg.mozilla.org/integration/mozilla-inbound/rev/312f7e5a89ce Should this have tests?
Flags: in-testsuite?
Keywords: checkin-needed
Assignee | ||
Comment 15•10 years ago
|
||
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.
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/028dbae2c819 https://hg.mozilla.org/mozilla-central/rev/710e2f5dca1c https://hg.mozilla.org/mozilla-central/rev/bdf66d134585 https://hg.mozilla.org/mozilla-central/rev/c0e3b8b4e441 https://hg.mozilla.org/mozilla-central/rev/b16e90588530 https://hg.mozilla.org/mozilla-central/rev/312f7e5a89ce
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•