Closed Bug 1298728 Opened 3 years ago Closed 3 years ago

Annotate some non-obvious case fallthroughs in the HTML parser.

Categories

(Core :: DOM: HTML Parser, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: njn, Assigned: njn)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [CID 748955, CID 748956])

Attachments

(1 file, 1 obsolete file)

nsHtml5TreeBuilder has a couple of case fallthroughs that Coverity finds suspicious. We should make the intent clearer here.
I have optimistically assumed that the current code is correct, and this patch
is just making it clearer. Please check that assumption! I'd be happy if this
had discovered a genuine bug.
Attachment #8785758 - Flags: review?(hsivonen)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
I missed one case.

I also now see that this is a generated file. hsivonen can you please check
whether these fallthroughs are ok? If so, then I'll just mark the bug WONTFIX
and mark them as false positives in Coverity. (If not, we can work out how to
fix the Java code.)
Attachment #8785759 - Flags: feedback?(hsivonen)
Attachment #8785758 - Attachment is obsolete: true
Attachment #8785758 - Flags: review?(hsivonen)
Comment on attachment 8785759 [details] [diff] [review]
Annotate some non-obvious case fallthroughs in the HTML parser

Indeed, this is generated code, so these annotations would be overwritten. Hence, feedback-.

All these cases are documented as comments in the Java source. We should probably change those comments to something like
// CPPONLY: fallThrough();
and then change the translator to special case a method invocation fallThrough() to emit C++ macro invocation that we could then define as expanding to whatever incantations each compiler or static analysis tool wants to see. ("// CPPONLY:" already gets removed by a preprecessor before translation, so that kind of Java comments allow Java code seen by the translator but not seen by the normal Java compiler.)
Attachment #8785759 - Flags: feedback?(hsivonen) → feedback-
Note that there are lots of intentional (commented in the Java source) fall-throughs in the tokenizer. Hence, https://dxr.mozilla.org/mozilla-central/source/parser/html/moz.build#104
> All these cases are documented as comments in the Java source. We should
> probably change those comments to something like
> // CPPONLY: fallThrough();
> and then change the translator to special case a method invocation
> fallThrough() to emit C++ macro invocation that we could then define as
> expanding to whatever incantations each compiler or static analysis tool
> wants to see.

I don't think that's worth the effort. I've marked the reports in Coverity as intentional. Thank you for the feedback.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.