doctype error incorrectly flagged for srcdoc URLs

RESOLVED FIXED in mozilla26

Status

()

Core
HTML: Parser
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jkitch, Assigned: jkitch)

Tracking

unspecified
mozilla26
x86_64
Windows 8
Points:
---

Firefox Tracking Flags

(firefox24 unaffected, firefox25 affected, firefox26 affected)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 800136 [details]
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → jkitch.bug
Component: View Source → HTML: Parser
Product: Toolkit → Core
(Assignee)

Comment 1

4 years ago
Created attachment 800751 [details] [diff] [review]
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.
Attachment #800751 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

4 years ago
Created attachment 800753 [details] [diff] [review]
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?
Attachment #800753 - Flags: review?(hsivonen)
(Assignee)

Updated

4 years ago
status-firefox24: --- → unaffected
status-firefox25: --- → affected
status-firefox26: --- → affected

Updated

4 years ago
QA Contact: alexandra.lucinet
Comment on attachment 800751 [details] [diff] [review]
Part 1 View-Source Channel changes

>+   *aIsSrcdocChannel = mIsSrcdocChannel;

Please fix the indent?

r=me
Attachment #800751 - Flags: review?(bzbarsky) → review+
For part 2, seems like we may want an "IsSrcdocChannel" helper in netutil or something..
(Assignee)

Comment 5

4 years ago
Created attachment 801219 [details] [diff] [review]
part 1: netwerk changes

The isSrcdocChannel check now resides within nsNetUtil.h
Attachment #800751 - Attachment is obsolete: true
Attachment #801219 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

4 years ago
Created attachment 801220 [details] [diff] [review]
part 2: parser changes

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

The question of testing (see comment 2) still applies.
Attachment #800753 - Attachment is obsolete: true
Attachment #800753 - Flags: review?(hsivonen)
Attachment #801220 - Flags: review?(hsivonen)
Attachment #801220 - Flags: review?(hsivonen) → review+
Comment on attachment 801219 [details] [diff] [review]
part 1: netwerk changes

r=me
Attachment #801219 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

4 years ago
Attachment #801220 - Attachment description: parser → part 2: parser changes
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e473e727e96
https://hg.mozilla.org/integration/mozilla-inbound/rev/e77fba8de25f
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9e473e727e96
https://hg.mozilla.org/mozilla-central/rev/e77fba8de25f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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.
(Assignee)

Comment 11

4 years ago
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?
Flags: needinfo?(Ms2ger)
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.
Flags: needinfo?(Ms2ger)
You need to log in before you can comment on or make changes to this bug.