Bug 40649 – Pull script line number from DocumentParser instead of pushing it to ScriptController

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Gentilcore]

Pull script line number from DocumentParser instead of pushing it to ScriptController

[Tony Gentilcore]

Created attachment 58848 [details]
Patch

[Early Warning System Bot]

Attachment 58848 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3337186

[Adam Barth]

Comment on attachment 58848 [details]
Patch

Nits below:

WebCore/bindings/js/ScriptEventListener.cpp:74
 +          if (DocumentParser* parser = frame->document()->parser())
Wow, so much prettier with the new names.

WebCore/ChangeLog:10
 +          cleaner IMHO.
Can you measure how much this helps us?  I'm curious what kind of scale change we need per token to move the benchmark.  I'm pretty sure the benchmark moves per-branch per-character.

WebCore/bindings/js/ScriptEventListener.cpp:75
 +              lineNumber = parser->lineNumber() + 1;
Can you make this a static inline function just so we can give this wacky conversion a name?

WebCore/bindings/v8/ScriptEventListener.cpp:68
 +              lineNumber = parser->lineNumber();
No +1 here?

[Eric Seidel (no email)]

How did it end up as push in the first place?  Can we CC the folks involved in that change in case there is knowledge to be shared?

[Eric Seidel (no email)]

Wow, nevermind.  This backwards design has been with us since back in the KJSProxy days.  Let's torch it. :)  I went back to r11588 and still found this stuff here.

[Eric Seidel (no email)]

Comment on attachment 58848 [details]
Patch

I'm changing this to a r-.  I'd like to see perf numbers from this change (from running our parser benchmark) and I would like to see the lineNumber+1 hack encapsulated into some sort of function somewhere.  Probably just moved over into the ScriptController code.  If it had a eventHandlerlineNumber() accessor which knew how to call the parser and do the +1, that seems simplest.

[Eric Seidel (no email)]

Oh, and if we're fixing a bug with event handler line numbers and the xml parser we should be sure to test that.  I can help you write the xhtml or svg if needed.

[Tony Gentilcore]

Created attachment 59014 [details]
Patch

[WebKit Review Bot]

Attachment 59014 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tony Gentilcore]

(In reply to comment #3)
> (From update of attachment 58848 [details])
> Nits below:
> 
> WebCore/bindings/js/ScriptEventListener.cpp:74
>  +          if (DocumentParser* parser = frame->document()->parser())
> Wow, so much prettier with the new names.

Yeah, nice work.

> 
> WebCore/ChangeLog:10
>  +          cleaner IMHO.
> Can you measure how much this helps us?  I'm curious what kind of scale change we need per token to move the benchmark.  I'm pretty sure the benchmark moves per-branch per-character.
> 

Here are my results:

Before patch:
avg 3948.5
stdev 30.445853576472445
avg 3946.25
stdev 28.035468606748843

After patch:
avg 3861.7
stdev 28.241990014869703
avg 3859.4
stdev 28.793054718108667

> WebCore/bindings/js/ScriptEventListener.cpp:75
>  +              lineNumber = parser->lineNumber() + 1;
> Can you make this a static inline function just so we can give this wacky conversion a name?
>

Done.
 
> WebCore/bindings/v8/ScriptEventListener.cpp:68
>  +              lineNumber = parser->lineNumber();
> No +1 here?

This matches the existing code. Adding the +1 makes the line number expectations in existing LayoutTests fail.

[Tony Gentilcore]

Created attachment 59017 [details]
Fix tab in ChangeLog

[Eric Seidel (no email)]

(In reply to comment #10)
> > WebCore/bindings/v8/ScriptEventListener.cpp:68
> >  +              lineNumber = parser->lineNumber();
> > No +1 here?
> 
> This matches the existing code. Adding the +1 makes the line number expectations in existing LayoutTests fail.

Current LayoutTest expectations aren't necessarily right, especially when it comes to line numbers for scripts.  if this is a V8 vs. JSC difference we should abstract it away using ScriptController somehow.  Parser line numbers need to be translated to "script engine" line numbers in some way, and that shouldn't be by manual +1's in random places. :)

[Eric Seidel (no email)]

Comment on attachment 59017 [details]
Fix tab in ChangeLog

Wow, this patch is GORGEOUS.

I would kinda like a comment here to explain the +1.  At least we only have one +1 now though:
+    if (DocumentParser* parser = m_frame->document()->parser())
+        return parser->lineNumber() + 1;

Also, do we need some sort of XML-based event handler test to test the fixed line number handling for the XML parser?

[Tony Gentilcore]

Created attachment 59067 [details]
Patch

[Tony Gentilcore]

Created attachment 59069 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 59069 [details]
Patch

Fantastic!

We should file a follow-up bug about passing your new test.

Thanks!

[Tony Gentilcore]

(In reply to comment #16)
> (From update of attachment 59069 [details])
> Fantastic!
> 
> We should file a follow-up bug about passing your new test.

https://bugs.webkit.org/show_bug.cgi?id=40809
> 
> Thanks!

[WebKit Commit Bot]

Comment on attachment 59069 [details]
Patch

Rejecting patch 59069 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Last 500 characters of output:
rtree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
Testing 19160 test cases.
svg/dom/script-line-number.xhtml -> failed

Exiting early after 1 failures. 16783 tests run.
326.66s total testing time

16782 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
15 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/3301412

[Tony Gentilcore]

(In reply to comment #18)
> (From update of attachment 59069 [details])
> Rejecting patch 59069 from commit-queue.
> 
> Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
> Last 500 characters of output:
> rtree
> Compiling Java tests
> make: Nothing to be done for `default'.
> Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
> Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
> Testing 19160 test cases.
> svg/dom/script-line-number.xhtml -> failed
> 
> Exiting early after 1 failures. 16783 tests run.
> 326.66s total testing time
> 
> 16782 test cases (99%) succeeded
> 1 test case (<1%) had incorrect layout
> 15 test cases (<1%) had stderr output
> 
> Full output: http://webkit-commit-queue.appspot.com/results/3301412

Is it possible to know which platform generated the failure? I'm having trouble reproducing locally.

[Eric Seidel (no email)]

The commit-queue runs on Mac Leopard.

[Tony Gentilcore]

(In reply to comment #20)
> The commit-queue runs on Mac Leopard.

I finally figured it out. It reports different line numbers when run after another test vs. being run by itself.

Something is really wacky with the line number from libxml2. I have two options:
1. Disable this new test in this patch
2. Fix the XML line numbers in this patch

[Eric Seidel (no email)]

We're probablyl not resetting libxml's internal state in between uses.  You should feel free to skip the test (or make it simply not print the line numbers in the expectations).  Or you could also figure out how to reset libxml's internal state.  Thank you for chasing the goose, but don't feel obligated to run too far. :)

[Eric Seidel (no email)]

We seem to use:
int XMLDocumentParser::lineNumber() const
{
    return context() ? context()->input->line : 1;
}

int XMLDocumentParser::columnNumber() const
{
    return context() ? context()->input->col : 1;
}

instead of:
http://xmlsoft.org/html/libxml-SAX2.html#xmlSAX2GetLineNumber

Not sure why.

It's possible that context->input gets re-used between contexts.  I kinda doubt it though.

[Eric Seidel (no email)]

I suggest just skipping on libxml platforms.  This stuff is complicated and broken.  If you'd like to dive into libXML2 feel free, but I've been there.  It's a trail of tears.  Mostly we end up having to be careful because libxml2 keeps a lot of global state, and clients of WebKit may also be clients of libxml.  When we've talked with DV (the author of libxml2) he's expressly discouraged using libxml2 from a library like we do.  So we have to hack around various bits of libxml2, like its encoding detection, because we only use it for its xml parsing, and not for all the http fetching, encoding detection, tree building, etc. that it has built into it.

[Tony Gentilcore]

(In reply to comment #24)
> I suggest just skipping on libxml platforms.  This stuff is complicated and broken.  If you'd like to dive into libXML2 feel free, but I've been there.  It's a trail of tears.  Mostly we end up having to be careful because libxml2 keeps a lot of global state, and clients of WebKit may also be clients of libxml.  When we've talked with DV (the author of libxml2) he's expressly discouraged using libxml2 from a library like we do.  So we have to hack around various bits of libxml2, like its encoding detection, because we only use it for its xml parsing, and not for all the http fetching, encoding detection, tree building, etc. that it has built into it.

Well context->input->line, context->node->line, and xmlSAX2GetLineNumber(context) all seem to behave identically. I also tried setting context->linenumbers = 1 and lineNumbersDefault(1) during initialization. So I'm giving up on chasing this goose.

Since libxml2 is used everywhere except QT, this would have to be skipped everywhere except there. I don't have a convenient way to build and test QT to see if the test works there, so I just commented out the expectations in the layout test and linked to https://bugs.webkit.org/show_bug.cgi?id=40809.

[Tony Gentilcore]

Created attachment 59204 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 59204 [details]
Patch

This si fine.  Seems silly land LayoutTests/svg/dom/script-line-number.xhtml which does nothing.  would be better to just attach it to the bug which is going to fix it instead. :)

[Tony Gentilcore]

Created attachment 59377 [details]
Patch

[Tony Gentilcore]

(In reply to comment #27)
> (From update of attachment 59204 [details])
> This si fine.  Seems silly land LayoutTests/svg/dom/script-line-number.xhtml which does nothing.  would be better to just attach it to the bug which is going to fix it instead. :)

Okay, I've removed the SVG test. I'm attaching it to https://bugs.webkit.org/show_bug.cgi?id=40809 now.

[Eric Seidel (no email)]

Comment on attachment 59377 [details]
Patch

You can just re-upload with "land-safely" which will queue it for the commit-queue (or you can mark the patch yourself).  If the reviewer field is filled in, you don't need another review, just cq+ (which now that you're a committer you can give yourself).

[Adam Barth]

Committed r61640: <http://trac.webkit.org/changeset/61640>

[WebKit Review Bot]

http://trac.webkit.org/changeset/61640 might have broken Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/61640
http://trac.webkit.org/changeset/61638
http://trac.webkit.org/changeset/61639

[Tony Gentilcore]

Created attachment 59453 [details]
Patch