358325 - Clear up the blocking API

Status: NEW

(Core :: DOM: HTML Parser, defect, P5)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We should document what the proper blocking API for the html parser is, and make sure to follow it. Currently it looks like the HTMLContentSink simply returns NS_ERROR_HTMLPARSER_BLOCK and assumes that that's enough to block the parser, whereas the nsXMLContentSink calls BlockParser() under certain circumstances.

Additionally, both sinks check if IsParserEnabled() returns false after executing script, and if so explicitly returns NS_ERROR_HTMLPARSER_BLOCK, is that needed?

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #0)
> We should document what the proper blocking API for the html parser is, and
> make sure to follow it. Currently it looks like the HTMLContentSink simply
> returns NS_ERROR_HTMLPARSER_BLOCK and assumes that that's enough to block the
> parser, whereas the nsXMLContentSink calls BlockParser() under certain
> circumstances.

The XML content sink cannot propagate its return values back to nsParser -- the driver discards the return values, therefore it must explicitly call BlockParser to tell the non-Expat side of things what's going on. The HTMLContentSink is called by CNavDTD, which explicitly does propagate the return value back to the parser, so it can rely on the parser blocking itself.

> Additionally, both sinks check if IsParserEnabled() returns false after
> executing script, and if so explicitly returns NS_ERROR_HTMLPARSER_BLOCK, is
> that needed?

At the very least, that return is needed to tell the DTD to not process more tokens until the parser is ready again. I *think* that the check is needed for nested parser invocations, where an inline script loads another script (or a remote stylesheet) that causes the parser to block, landing us in a case where we don't think we need to block the parser, even though the parser is already blocked.

Hmm, looking at the null check in the HTML content sink (mParser && mParser->IsParserEnabled()), I think that null check causes us to return NS_OK when we want to block the parser -- we want the DTD to stop processing tokens if the parser is no longer parser, don't we?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> > We should document what the proper blocking API for the html parser is, and
> > make sure to follow it. Currently it looks like the HTMLContentSink simply
> > returns NS_ERROR_HTMLPARSER_BLOCK and assumes that that's enough to block 
> > the parser, whereas the nsXMLContentSink calls BlockParser() under certain
> > circumstances.
> 
> The XML content sink cannot propagate its return values back to nsParser -- 
> the driver discards the return values, therefore it must explicitly call
> BlockParser to tell the non-Expat side of things what's going on. The
> HTMLContentSink is called by CNavDTD, which explicitly does propagate the
> return value back to the parser, so it can rely on the parser blocking itself.

Could we make nsExpatDriver make this check rather than having to have the sink do it?

> > Additionally, both sinks check if IsParserEnabled() returns false after
> > executing script, and if so explicitly returns NS_ERROR_HTMLPARSER_BLOCK, is
> > that needed?
> 
> At the very least, that return is needed to tell the DTD to not process more
> tokens until the parser is ready again. I *think* that the check is needed for
> nested parser invocations, where an inline script loads another script (or a
> remote stylesheet) that causes the parser to block, landing us in a case where
> we don't think we need to block the parser, even though the parser is already
> blocked.

My gut feeling is that it would be cleaner if the DTD checked this and acted accordingly. The sink should not have to care about the inner workings of the DTD. However It's not a big deal unless we create more sinks that needs to do blocking and such.

> Hmm, looking at the null check in the HTML content sink (mParser &&
> mParser->IsParserEnabled()), I think that null check causes us to return NS_OK
> when we want to block the parser -- we want the DTD to stop processing tokens
> if the parser is no longer parser, don't we?

Does it matter? When do mParser really become null?

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #2)
> Could we make nsExpatDriver make this check rather than having to have the sink
> do it?

Sure.

> My gut feeling is that it would be cleaner if the DTD checked this and acted
> accordingly. The sink should not have to care about the inner workings of the
> DTD. However It's not a big deal unless we create more sinks that needs to do
> blocking and such.

Actually, except for the parser interruption stuff, the DTD is currently blissfully unaware of parser blocking. I would rather leave things the way they are, especially since this case only arises if you do something synchronously that can block the parser, which seems rare to me.

> Does it matter? When do mParser really become null?

The check was added for bug 88386. I haven't tested to see if it's still necessary.

Comment 4

[Jens Stutte [:jstutte]]

Bulk-downgrade of unassigned, >=5 years untouched DOM/Storage bugs' priority.

If you have reason to believe this is wrong (especially for the severity), please write a comment and ni :jstutte.