912981 - doctype error incorrectly flagged for srcdoc URLs

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[James Kitchener (:jkitch)]

Attachment: test.html

Load the attached file, open the context menu for the iframe, select "This Frame" and "View Frame Source"

Actual result:

The <p> tag is highlighted in red because a missing doctype error is reported.

Expected result:

No error should be reported.
The doctype element is optional within srcdoc iframes.

Comment 1

[James Kitchener (:jkitch)]

Attachment: Part 1 View-Source Channel changes

It turns out users of nsIViewSourceChannels need to be aware of the srcdoc status of the embedded channel.

I'm not extending knowledge of this status to nsIDocument because I haven't discovered a situation where it matters.  It may also cause unintended side-effects.

Comment 2

[James Kitchener (:jkitch)]

Attachment: Part 2: parser changes

Need to check nsIViewSourceChannel as well as nsIInputStreamChannel.

I have tested this locally, but does this need an automated test?  If so, could you provide some guidance how to perform the test or alternatively point out other tests which assess parser error reporting?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 800751 [details] [diff] [review]
Part 1 View-Source Channel changes

>+   *aIsSrcdocChannel = mIsSrcdocChannel;

Please fix the indent?

r=me

Comment 4

[Boris Zbarsky [:bzbarsky]]

For part 2, seems like we may want an "IsSrcdocChannel" helper in netutil or something..

Comment 5

[James Kitchener (:jkitch)]

Attachment: part 1: netwerk changes

The isSrcdocChannel check now resides within nsNetUtil.h

Comment 6

[James Kitchener (:jkitch)]

Attachment: part 2: parser changes

This moves the test for srcdoc status out of the parser.

The question of testing (see comment 2) still applies.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 801219 [details] [diff] [review]
part 1: netwerk changes

r=me

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e473e727e96
https://hg.mozilla.org/integration/mozilla-inbound/rev/e77fba8de25f

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9e473e727e96
https://hg.mozilla.org/mozilla-central/rev/e77fba8de25f

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 801219 [details] [diff] [review]
part 1: netwerk changes

Review of attachment 801219 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/public/nsNetUtil.h
@@ +2345,5 @@
> +{
> +  bool isSrcdoc;
> +  nsCOMPtr<nsIInputStreamChannel> isr = do_QueryInterface(aChannel);
> +  if (isr) {
> +    isr->GetIsSrcdocChannel(&isSrcdoc);

This looks unsafe in general... I don't see a reason to assume that this initializes isSrcdoc.

Comment 11

[James Kitchener (:jkitch)]

Could you explain your reasoning?  I cannot see the problem.

If the channel is neither an nsIInputStreamChannel or a nsIViewSourceChannel, isSrcdoc isn't used and a constant false is returned.

If you mean that the value returned by GetIsSrcdocChannel(bool *aIsSrcdocChannel), I believe that it is initialised on object creation.

>     nsInputStreamChannel() :
>       mIsSrcdocChannel(false) {}

Are there other ways that an nsInputStreamChannel can be created?

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

GetIsSrcdocChannel is an xpcom method, so it is not required to actually set its outparam if it returns a failure nsresult. Since you don't check the result, you can't be sure that that hasn't happened.