Bug 156541 - Fixed crash in PrettyPatch.
Summary: Fixed crash in PrettyPatch.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-13 09:25 PDT by Konstantin Tokarev
Modified: 2017-07-18 08:27 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.16 KB, patch)
2016-04-13 09:28 PDT, Konstantin Tokarev
achristensen: review+
dbates: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Konstantin Tokarev 2016-04-13 09:25:12 PDT
PrettyPatch crashed when parsing https://bugs.webkit.org/attachment.cgi?id=276320
Comment 1 Konstantin Tokarev 2016-04-13 09:28:30 PDT
Created attachment 276328 [details]
Patch
Comment 2 Alex Christensen 2016-04-13 11:40:29 PDT
This looks like it might be good.  Sorry for my unfamiliarity with these tests, but how do I run them?
Comment 3 Konstantin Tokarev 2016-04-13 11:47:03 PDT
I've run it with

ruby -IWebsites/bugs.webkit.org/PrettyPatch Websites/bugs.webkit.org/PrettyPatch/PrettyPatch_test.rb 

4 tests were failing before my patch already
Comment 4 Alex Christensen 2016-04-13 11:51:16 PDT
Thanks, this certainly fixes the crash, but does it correctly process the patch?
Comment 5 Konstantin Tokarev 2016-04-13 12:19:27 PDT
No. It actually parses '--' in header as a removal mark
Comment 6 Daniel Bates 2016-04-13 12:22:20 PDT
Do we have crash report? If this is a crash then is there a Ruby bug for it?
Comment 7 Konstantin Tokarev 2016-04-13 12:26:54 PDT
This is not crash in ruby interpreter, only uncaught exception in script
Comment 8 Daniel Bates 2016-04-13 12:35:00 PDT
(In reply to comment #7)
> This is not crash in ruby interpreter, only uncaught exception in script

Then "crash" is not correct word to use here. Please update both the bug title of this Bugzilla bug and ChangeLog bug title and description to convey that this fix is for an exception - even better include the name of the exception. If you do not include the name of the exception thrown in the bug title then please explain in English the cause of the exception in the ChangeLog description.
Comment 9 Csaba Osztrogonác 2016-05-10 07:37:06 PDT
Could you rephrase the changelog and land it?
Comment 10 Konstantin Tokarev 2016-05-10 07:58:00 PDT
Actually, I've found that it's not the proper solution to the problem (see comment #5). Script does not bail out anymore, but highlighting can also be fixed.

Have you stumbled upon another "evil" patch? If this is the case I'll reword and submit it ASAP
Comment 11 Csaba Osztrogonác 2016-05-10 08:03:46 PDT
Nevermind. I only read Comment 8, not Comment 5.