Bug 135704 - JSC Lexer is allowing octals 08 and 09 in strict mode functions
Summary: JSC Lexer is allowing octals 08 and 09 in strict mode functions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
URL: https://bugs.ecmascript.org/show_bug....
Keywords: EasyFix, InRadar
: 126618 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-08-07 10:05 PDT by Oliver Hunt
Modified: 2014-08-10 13:15 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.78 KB, patch)
2014-08-08 08:53 PDT, Diego Pino
no flags Details | Formatted Diff | Diff
Patch (3.66 KB, patch)
2014-08-08 12:56 PDT, Diego Pino
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (485.87 KB, application/zip)
2014-08-08 14:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (503.77 KB, application/zip)
2014-08-08 14:22 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (507.54 KB, application/zip)
2014-08-08 15:19 PDT, Build Bot
no flags Details
Patch (5.27 KB, patch)
2014-08-09 04:56 PDT, Diego Pino
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2014-08-07 10:05:56 PDT
(function() { "use strict"; return 08.9 })()

This should be a syntax error.
Comment 1 Radar WebKit Bug Importer 2014-08-07 10:06:16 PDT
<rdar://problem/17946715>
Comment 2 Oliver Hunt 2014-08-07 10:09:06 PDT
*** Bug 126618 has been marked as a duplicate of this bug. ***
Comment 3 Mathias Bynens 2014-08-07 11:58:02 PDT
(In reply to comment #0)
> (function() { "use strict"; return 08.9 })()
> 
> This should be a syntax error.

For future reference: as per the current spec, it should be a syntax error even in non-strict mode, but unfortunately that ship has sailed. See http://esdiscuss.org/topic/early-error-on-0-followed-by-8-or-9-in-numeric-literals-does-not-seem-to-be-web-compatible for the rationale on why banning it in strict mode might be a good idea for now.
Comment 4 Diego Pino 2014-08-08 08:53:46 PDT
Created attachment 236280 [details]
Patch
Comment 5 Diego Pino 2014-08-08 08:55:02 PDT
Sorry Oliver Hunt, I didn't realized it was assigned to you.
Comment 6 Oliver Hunt 2014-08-08 10:07:05 PDT
Comment on attachment 236280 [details]
Patch

r=me, but we could you add a test case? easiest is probably to add it to js/parser-syntax-check .  Thanks for the patch :)
Comment 7 Mathias Bynens 2014-08-08 11:18:17 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=236280&action=review

> Source/JavaScriptCore/parser/Lexer.cpp:1668
> +            m_lexErrorMessage = "Octal escapes are forbidden in strict mode";

`08` and `09` are not octal escapes, since `8` and `9` are not octal digits.

Better error message (+ commit message):

“Decimal integer literals with a leading zero are forbidden in strict mode”
Comment 8 Diego Pino 2014-08-08 12:56:54 PDT
Created attachment 236299 [details]
Patch
Comment 9 Oliver Hunt 2014-08-08 13:28:58 PDT
Thanks!
Comment 10 Mathias Bynens 2014-08-08 13:46:43 PDT
(In reply to comment #8)
> Created an attachment (id=236299) [details]
> Patch

Commit message (first line) still incorrectly mentions “octals”.
Comment 11 Build Bot 2014-08-08 14:02:10 PDT
Comment on attachment 236299 [details]
Patch

Attachment 236299 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6206489106579456

New failing tests:
js/basic-strict-mode.html
Comment 12 Build Bot 2014-08-08 14:02:13 PDT
Created attachment 236302 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 13 Build Bot 2014-08-08 14:22:05 PDT
Comment on attachment 236299 [details]
Patch

Attachment 236299 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5176058806534144

New failing tests:
js/basic-strict-mode.html
Comment 14 Build Bot 2014-08-08 14:22:09 PDT
Created attachment 236305 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 15 Build Bot 2014-08-08 15:19:50 PDT
Comment on attachment 236299 [details]
Patch

Attachment 236299 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6012677633605632

New failing tests:
js/basic-strict-mode.html
Comment 16 Build Bot 2014-08-08 15:19:54 PDT
Created attachment 236315 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 17 Diego Pino 2014-08-08 15:20:50 PDT
Apparently there's a test for octal numbers in strict mode in js/basic-strict-mode. Since I have to resend the patch, should I move the tests I added to js/parser-syntax-check-expected to js/basic-strict-mode?
Comment 18 Diego Pino 2014-08-08 15:24:50 PDT
(In reply to comment #10)
> (In reply to comment #8)
> > Created an attachment (id=236299) [details] [details]
> > Patch
> 
> Commit message (first line) still incorrectly mentions “octals”.

Generally the first line of the commit message matches the title of the bug in the bugzilla. I think is better not to modify the commit message unless the title of the bug is modified too.
Comment 19 Oliver Hunt 2014-08-08 15:25:47 PDT
(In reply to comment #17)
> Apparently there's a test for octal numbers in strict mode in js/basic-strict-mode. Since I have to resend the patch, should I move the tests I added to js/parser-syntax-check-expected to js/basic-strict-mode?

fix the basic-strict-mode test, and leave your new tests in the parser-syntax-check test
Comment 20 Diego Pino 2014-08-09 04:56:50 PDT
Created attachment 236333 [details]
Patch
Comment 21 WebKit Commit Bot 2014-08-10 13:07:52 PDT
Comment on attachment 236333 [details]
Patch

Clearing flags on attachment: 236333

Committed r172380: <http://trac.webkit.org/changeset/172380>
Comment 22 WebKit Commit Bot 2014-08-10 13:07:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Oliver Hunt 2014-08-10 13:15:21 PDT
Thanks!