483015 - [HTML5] Expose HTML line number to JS and CSS parsers

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[Henri Sivonen (:hsivonen)]

The tokenizer needs to keep count of the line number in order to avoid regressing CSS and JS error reporting.

Comment 1

[Henri Sivonen (:hsivonen)]

Avoid regressing bug 190576.

Comment 2

[Henri Sivonen (:hsivonen)]

Fixed in rev cbd95aa84038 in the HTML5 repo.

Comment 3

[Henri Sivonen (:hsivonen)]

Forgot the case where doc.write leaves data in the input stream when returning.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

This doesn't seem to work at all for <script> or <style>, in my testing.  Testcase attached

Comment 5

[Henri Sivonen (:hsivonen)]

Attachment: Fix

This patch should give sensible line numbers for content in the network stream and for content in the root context of document.open()ed documents.

Line numbers in other document.written content won't necessarily match the line numbers reported by the old HTML parser.

Comment 6

[Henri Sivonen (:hsivonen)]

Attachment: Mochitest

Comment 7

[Henri Sivonen (:hsivonen)]

Attachment: Right line numbers for network and document.opened root context, approximate for other document.writes

Per Boris' comments in .platform, I've retained approximate line numbers even when they are know to be bogus.

Moving the line number in and out of the tokenizer in nsHtml5Parser is quite ugly, but I didn't want the tokenizer to know about all this.

Comment 8

[Henri Sivonen (:hsivonen)]

Attachment: Refreshed mochitest

Comment 9

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 411666 [details] [diff] [review]
Right line numbers for network and document.opened root context, approximate for other document.writes

Looks good, and playing around with the testcase produced all the right behaviors, as far as I can see.

Comment 10

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 411667 [details] [diff] [review]
Refreshed mochitest

I'd love to see a few more tests here.  What happens with multiple unknown properties? Multiple <style> tags?

Comment 11

[Henri Sivonen (:hsivonen)]

Attachment: Test two <style> tags, two bogus properties

(In reply to comment #9)
> (From update of attachment 411666 [details] [diff] [review])
> Looks good, and playing around with the testcase produced all the right
> behaviors, as far as I can see.

Thanks.

(In reply to comment #10)
> (From update of attachment 411667 [details] [diff] [review])
> I'd love to see a few more tests here.  What happens with multiple unknown
> properties? Multiple <style> tags?

Attached mochitests that have two <style> tags each and also test cases where the error isn't on the first line of the <style> element.

Comment 12

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 414031 [details] [diff] [review]
Test two <style> tags, two bogus properties

r=me provided you add the code to set/reset the html5.enable pref

Comment 13

[Henri Sivonen (:hsivonen)]

(In reply to comment #12)
> (From update of attachment 414031 [details] [diff] [review])
> r=me provided you add the code to set/reset the html5.enable pref

Thanks.

(I haven't toggled the pref on test cases that are supposed to pass with both parsers, but I'll change it here. To get good test coverage anyway, it's necessary to push to try with html5.enable set to true.)

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

a=beltzner to land on mozilla-central as this is pref'd off by default and so I'm happy to consider it NPOTB for now.

Comment 15

[Henri Sivonen (:hsivonen)]

Thanks for the a. Pushed:
http://hg.mozilla.org/mozilla-central/rev/60fea11d79cd

(I didn't push the test yet, since it depends on bug 529544.)

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 414031 [details] [diff] [review]
Test two <style> tags, two bogus properties

Random nits:

>+      ok(this.lineNumbers.length > 0, "Seen too many errors!");

Maybe this was copied from another test, but s/Seen/Saw/ for better English, or just "Too many errors!".

>+      
>+      var scriptErr = message.QueryInterface(Ci.nsIScriptError);
>+
>+      is(scriptErr.lineNumber, this.lineNumbers.shift(),
>+         "Wrong line");
>+    },
>+    
>+    finish: function() {
>+      ok(this.lineNumbers.length == 0 , "Didn't the right number of messages.");

Missing "get"? Same error later.

Great to have these line numbering bugs fixed!

/be

Comment 17

[Henri Sivonen (:hsivonen)]

(In reply to comment #16)
> (From update of attachment 414031 [details] [diff] [review])
> Random nits:

Nits fixed locally. Thanks.

I haven't pushed this fix, because flipping the html5.enable pref here makes some document that lives across tests (the harness itself presumably) cache a fragment parser and then the innerHTML setting fails in a subsequent test because the pref and the cached fragment parser are out of sync.

bnewman, is it OK if I make this test not flip the pref? sicking says making the document remember what kind of fragment parser it has is a waste of time.

(I don't have an explanation why this is a problem with this test case but not with other test cases that flip the pref.)