336881 – Unclosed comment errors with commented tags

Bug 336881 - Unclosed comment errors with commented tags

Summary: Unclosed comment errors with commented tags

Status:	VERIFIED FIXED

Alias:	None

Product:	WTP Source Editing
Classification:	WebTools
Component:	jst.jsp (show other bugs)
Version:	3.2.3
Hardware:	All All

Importance:	P1 major (vote)
Target Milestone:	3.2.3
Assignee:	Nick Sandonato
QA Contact:	Nick Sandonato

URL:
Whiteboard:	PMC_approved WI67955
Keywords:	performance

Depends on:
Blocks:

Reported:	2011-02-10 16:38 EST by Nick Sandonato
Modified:	2011-02-16 20:24 EST (History)
CC List:	5 users (show)

See Also:

Flags:	david_williams: pmc_approved+ raghunathan.srinivasan: pmc_approved+ thatnitind: pmc_approved? (naci.dai) thatnitind: pmc_approved? (deboer) neil.hauge: pmc_approved+ kaloyan: pmc_approved+ thatnitind: review+

Attachments
patch (12.63 KB, patch) 2011-02-10 16:43 EST, Nick Sandonato	no flags	Details \| Diff
unit tests (2.46 KB, patch) 2011-02-14 14:16 EST, Nick Sandonato	no flags	Details \| Diff
unit tests (one more) (2.95 KB, patch) 2011-02-14 14:52 EST, Nitin Dahyabhai	no flags	Details \| Diff
handling of comment with a tag (84.92 KB, image/png) 2011-02-14 17:06 EST, Nitin Dahyabhai	no flags	Details
handling of comment with an embedded tag (83.12 KB, image/png) 2011-02-14 17:06 EST, Nitin Dahyabhai	no flags	Details
error message shown for correct text (12.04 KB, image/png) 2011-02-14 17:06 EST, Nitin Dahyabhai	no flags	Details
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Nick Sandonato

2011-02-10 16:38:20 EST

In resolving Bug 326193, it seems like certain scenarios started springing up that would cause unclosed comment errors to be generated. The most recent one would occur for something like:

<!--<link HREF='<c:out value='localhost/file.css'/>'>-->

To correct this, we'll be backing out the changes made and employing a new strategy. Instead, we'll go back to relying on the XMLJSPRegionHelper to properly decode these commented regions.

Comment 1 Nick Sandonato

2011-02-10 16:43:35 EST

Created attachment 188723 [details]
patch

Tokenizer: changes reverted (probably a performance improvement too since we're not creating as many text regions (almost one was being created per character in comments) and the buffer was copied into a string each time a comment was scanned

XMLJSPRegionHelper: The contents of a comment are translated as a whole instead of by each region within. This is because the tokenizer would create a new text region separate from the start tag when it was included in a comment

Comment 2 Nick Sandonato

2011-02-14 14:16:29 EST

Created attachment 188934 [details]
unit tests

Added unit tests for the scenario outlined in the description to make sure that the comment is properly closed.

Additionally, a unit test has been added to cover another scenario that is paired with this "comment unclosed" issue, which was that if any markup was included in an HTML comment (e.g., <!-- <div></div> -->) each character was becoming its own text region, increasing memory usage and parsing time. This unit test covers to make sure that the comment remains a contiguous XML_COMMENT_TEXT.

Comment 3 Nitin Dahyabhai

2011-02-14 14:52:50 EST

Created attachment 188943 [details]
unit tests (one more)

Comment 4 Nitin Dahyabhai

2011-02-14 17:06:04 EST

Created attachment 188961 [details]
handling of comment with a tag

Comment 5 Nitin Dahyabhai

2011-02-14 17:06:27 EST

Created attachment 188962 [details]
handling of comment with an embedded tag

Comment 6 Nitin Dahyabhai

2011-02-14 17:06:49 EST

Created attachment 188963 [details]
error message shown for correct text

Comment 7 Nitin Dahyabhai

2011-02-15 17:51:03 EST

* Explain why you believe this is a stop-ship defect. Or, if it is a "hotbug" (requested by an adopter) please document it as such.
It's a functional and performance regression from the prior release.
* Is there a work-around? If so, why do you believe the work-around is insufficient?
There is no workaround.  It's a defect in our low-level tokenizer and an associated class.
* How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added?
Manually tested by Nick, also with the included unit tests written by Nick and myself.
* Give a brief technical overview. Who has reviewed this fix?
Changes how the parser handles tags within XML comments so that they don't affect that part of the state machine; a comment end will be recognized regardless of what precedes it, as before.
* What is the risk associated with this fix?
The affected area is wide-reaching, but we've lowered the risk as much as we can.  Low.

Comment 8 Raghunathan Srinivasan

2011-02-15 19:54:33 EST

This is definitely a stop-ship and must be fixed.

Comment 9 Nitin Dahyabhai

2011-02-16 09:58:50 EST

Verified with 3.2.3-20110216062908 candidate, both visually in the Outline and with our info dialog.

Comment 10 David Williams

2011-02-16 17:03:32 EST

assuming status should be "fixed"?

Comment 11 Nitin Dahyabhai

2011-02-16 20:24:29 EST

Yes