| Summary: | Assertion firing in JavaScriptCore/parser/parser.h for statesman.com site | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||
| Component: | JavaScriptCore | Assignee: | Geoffrey Garen <ggaren> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | bfulgham, commit-queue, ggaren, mark.lam, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Mac | ||||||||||
| OS: | OS X 10.10 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Brent Fulgham
2015-03-23 11:57:20 PDT
*** Bug 142973 has been marked as a duplicate of this bug. *** I think I caused this in <http://trac.webkit.org/changeset/181810>. It looks like the HTML parser sometimes -- including on this site -- produces crazy negative column numbers, and the sanitization that I relaxed in <http://trac.webkit.org/changeset/181810> used to protect us against them. I don't really like the solution I came up with for attribute event listeners before, so I'll take this opportunity to fix them up a bit. Also at http://www.theblaze.com/stories/2015/03/25/man-thought-he-had-to-worry-about-people-stealing-his-packages-but-was-surprised-by-what-he-caught-a-mail-carrier-doing-on-camera-instead/ (from bug 143054). Created attachment 249520 [details]
Patch
Attachment 249520 [details] did not pass style-queue:
ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 19 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 249520 [details]
Patch
Will the line number rebasing require changes to LayoutTests/inspector/debugger/{breakpoint-columns,breakpoint-scope,search-scripts} tests? These use line number in the tests and expected output.
> Will the line number rebasing require changes to
> LayoutTests/inspector/debugger/{breakpoint-columns,breakpoint-scope,search-
> scripts} tests? These use line number in the tests and expected output.
I didn't see any changes in those tests when I ran them.
For reasons that are subtle, the old way and the new way are equivalent for most line numbers -- it's just that the new way is more robust and catches some corner cases.
Created attachment 249527 [details]
Patch
Added a test specifically covering the behavior change I described in Comment 10. Created attachment 249528 [details]
Patch
Comment on attachment 249528 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249528&action=review r=me > Source/JavaScriptCore/interpreter/StackVisitor.cpp:337 > + if (codeBlock->ownerExecutable()->hasOverrideLineNo()) > + line = codeBlock->ownerExecutable()->overrideLineNo(); Are the divot details important? If you have a multiline inline attribute, will this provide the expected line for messages/errors for each line? Test: <div onclick="console.log(1); console.log(2); console.log(3);">Click Me</div> > Source/JavaScriptCore/runtime/Executable.h:363 > + void setOverrideLineNo(int overrideLineNo) { m_overrideLineNo = overrideLineNo; } > + bool hasOverrideLineNo() { return m_overrideLineNo != -1; } > + int overrideLineNo() { return m_overrideLineNo;; } Normally we avoid abbreviations. Especially weird ones like "No" instead of "Number", despite the pre-existing case, can we make the new versions "overrideLineNumber"? Nit: The bottom two look like they can be made const. Nit: Double semicolon on line 363. > Source/WebCore/bindings/js/JSLazyEventListener.cpp:109 > + // We want all errors to refer back to the line on which our attribute was > + // declared, regardless of any newlines in our JavaScript source text. > + int overrideLineNo = m_position.m_line.oneBasedInt(); Ahh, it seems your intent is to always be the line of the attribute and not just with an original offset starting from the the line of the attribute. That seems fine. > Are the divot details important? If you have a multiline inline attribute, > will this provide the expected line for messages/errors for each line? Test: > > <div onclick="console.log(1); > console.log(2); > console.log(3);">Click Me</div> > > Source/WebCore/bindings/js/JSLazyEventListener.cpp:109 > > + // We want all errors to refer back to the line on which our attribute was > > + // declared, regardless of any newlines in our JavaScript source text. > > + int overrideLineNo = m_position.m_line.oneBasedInt(); > > Ahh, it seems your intent is to always be the line of the attribute and not > just with an original offset starting from the the line of the attribute. > That seems fine. Right. It would be nicer to get the exact right line in this multiline case, but that's very tricky because: (1) Newlines in JavaScript source text do not always break a line in the .html file; (2) JavaScript anonymous function rules add their own newlines. So, I think the best we can do for these attributes (and it's pretty good) is just to guarantee that we'll point you back at the location of the attribute. > > Source/JavaScriptCore/runtime/Executable.h:363 > > + void setOverrideLineNo(int overrideLineNo) { m_overrideLineNo = overrideLineNo; } > > + bool hasOverrideLineNo() { return m_overrideLineNo != -1; } > > + int overrideLineNo() { return m_overrideLineNo;; } > > Normally we avoid abbreviations. Especially weird ones like "No" instead of > "Number", despite the pre-existing case, can we make the new versions > "overrideLineNumber"? Yes. I'll take care of this in a follow-up style patch. > Nit: The bottom two look like they can be made const. > Nit: Double semicolon on line 363. Fixed. Comment on attachment 249528 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249528&action=review r=me > Source/WebCore/bindings/js/ScriptController.cpp:281 > + // FIXME: If we are not currently parsing, we should use our current location > + // in JavaScript, to cover cases like "element.setAttribute('click', ...)". > + > + // FIXME: This location maps to the end of the HTML tag, and not to the > + // exact column number belonging to the event handler attribute. Add defects for these FIXME's. Committed r182034: <http://trac.webkit.org/changeset/182034> |