Community
Participate
Working Groups
Build Identifier: M20101201-0800 It does not happen always.Trying to edit a html file caused this error.Attaching error log and html file that caused this error. Reproducible: Sometimes
Created attachment 187722 [details] log
Created attachment 187723 [details] file
I analyzed stack trace and came to following conclusion. 1)StackOverflow happens because variable 'refresh' is true in DOMModelImpl . 1-a)handleRefresh() function in this class checks this variable. 1-b)parser.replaceStructuredDocumentRegions() is called.It tries to add a comment element. 1-c)comment element sets attribute value, which again calls handleRefresh() and it leads to stackoverflow error. 2)'refresh' is set to true only if some exception happens.Although it is not clear from this stacktrace what exception caused this.I was able to reproduce this problem in debug mode by setting 'refresh' to true manually . All other exceptions in log followed, so i suspect this is the reason. We can get around this error if we set 'refresh' = false as a first statement in try block of handleRefresh() function instead of doing it in finally . Let me know if this is the correct analysis.
Created attachment 187809 [details] patch
Patch looks good to me and avoids a frustrating problem.
* 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. Stackoverflow renders the editor unusable. * Is there a work-around? If so, why do you believe the work-around is insufficient? No work around. * How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added? Ad hoc testing. * Give a brief technical overview. Who has reviewed this fix? When a model is refreshing, a result of the #replaceStructuredDocumentRegions() may cause handleRefresh() to be invoked again. In order to prevent this, we should be flagging the model as not needing refreshed. Rakesh's fix does this by setting the refresh flag to false before the call to #replaceStructuredDocumentRegions(). I have reviewed the patch. * What is the risk associated with this fix? Low. Just guarding from the stack overflow.
This certainly sounds worthy of a fix, so I'm in favor of it, but I'd encourage you to test really well. And by that I mean test _other_ cases really well. Changing the refresh value sounds like a big change ... but ... I do not know the whole context of the code so it is probably a fine fix. Thanks,
Thank you for the reviews. I think we're safe preventing a refresh from kicking off another refresh. But we'll keep an eye on things for sure. Resolving.
