Update HTML parser Tokenizer.java and StackNode.java for bug 1453795

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
7 months ago

People

(Reporter: hsivonen, Assigned: jkt)

Tracking

({csectype-uninitialized, sec-audit})

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: [adv-main65-])

Attachments

(4 attachments, 8 obsolete attachments)

4.41 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
1.30 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.04 KB, patch
jkt
: review+
Details | Diff | Splinter Review
1.24 KB, patch
jkt
: review+
Details | Diff | Splinter Review
Bug 1453795 edited the generated files. Need to change the Java files so as to not undo the changes.
Group: core-security → dom-core-security
This looks like it no longer has to be hidden is that correct :hsivonen?

I just hit this along with Bug 1483458 whilst trying to update the parser.
Flags: needinfo?(hsivonen)
The following files are the only changes I see when I generate the code:

- parser/html/nsHtml5StackNode.cpp
- parser/html/nsHtml5Tokenizer.cpp
(In reply to Jonathan Kingston [:jkt] from comment #1)
> This looks like it no longer has to be hidden is that correct :hsivonen?

Correct, but I don't have the permission bits to do the unhiding.
Flags: needinfo?(hsivonen)
No longer security sensitive.
Group: dom-core-security
Assignee: nobody → jkt
Posted patch bug-1466449-parser.patch (obsolete) — Splinter Review
Attachment #9018867 - Flags: review?(hsivonen)
Posted patch bug-1466449.patch (obsolete) — Splinter Review
Hi :andi, After reimporting the changes back into the java code from your patch in Bug 1453795 I just wanted to make sure there are no implications to the following:

-  , additional(u'\0')
+  , additional('\0')


and

-  , endTagExpectationAsArray{}
+  , endTagExpectationAsArray(jArray<char16_t, int32_t>::newJArray(0))

Everything else from what I can tell is just code style changes, however I would appreciate more eyes on it too to verify.


Thanks
Attachment #9018868 - Flags: review?(hsivonen)
Attachment #9018868 - Flags: review?(bpostelnicu)
As I understand it the difference between '\0' and u'\0' is the length the former being utf8 and the latter being utf16. This shouldn't have an impact given that we are using utf16 here. The rest of the code is using '\0' for utf16 nulls which if this is an issue we have other issues elsewhere.

As I understand it endTagExpectationAsArray{} is the same as a blank newJArray too.
Attachment #9018867 - Attachment is obsolete: true
Attachment #9018867 - Flags: review?(hsivonen)
Attachment #9018869 - Flags: review?(hsivonen)
Posted patch bug-1466449.patch (obsolete) — Splinter Review
I had to fix the init order for the "-Wreorder" cpp check to be happy.
Attachment #9018868 - Attachment is obsolete: true
Attachment #9018868 - Flags: review?(hsivonen)
Attachment #9018868 - Flags: review?(bpostelnicu)
Attachment #9018870 - Flags: review?(hsivonen)
Attachment #9018870 - Flags: review?(bpostelnicu)
Blocks: 903372
Status: NEW → ASSIGNED
Comment on attachment 9018870 [details] [diff] [review]
bug-1466449.patch

Review of attachment 9018870 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the HTML parser parts, but r- for the reformatting of unrelated files in m-c.
Attachment #9018870 - Flags: review?(hsivonen) → review-
Comment on attachment 9018869 [details] [diff] [review]
bug-1466449-parser.patch

Review of attachment 9018869 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #9018869 - Flags: review?(hsivonen) → review+
Posted patch bug-1466449.patch (obsolete) — Splinter Review
Removed formatting for android files.
Attachment #9018870 - Attachment is obsolete: true
Attachment #9018870 - Flags: review?(bpostelnicu)
Attachment #9018993 - Flags: review?(bpostelnicu)
Comment on attachment 9018993 [details] [diff] [review]
bug-1466449.patch

Looks good to me, thank you. Still I'm curious why didn't you choose the member list initialization for most most of the fields?
Attachment #9018993 - Flags: review?(bpostelnicu) → review+
> Still I'm curious why didn't you choose the member list initialization for most most of the fields?

I tried this first and it didn't seem to make any difference to the generated cpp code.

Thanks for the reviews :)
bug-1466449-parser.patch - needs to be checked into the html parser: https://hg.mozilla.org/projects/htmlparser/
bug-1466449.patch - should be checked into mozilla-central/inbound etc
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb24d2d42554
Update Tokenizer.java and StackNode.java to initialize properties. r=andi
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cb24d2d42554
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Are you able to check in the parser changes again? I'm not sure if there is a process for doing these patches?

Thanks!
Flags: needinfo?(hsivonen)
Pushed to the htmlparser repo before I saw the backout:
https://hg.mozilla.org/projects/htmlparser/rev/94eac97d452e
Flags: needinfo?(jkt)
Comment on attachment 9018993 [details] [diff] [review]
bug-1466449.patch

Review of attachment 9018993 [details] [diff] [review]:
-----------------------------------------------------------------

::: parser/html/javasrc/Tokenizer.java
@@ +551,4 @@
>          this.bmpChar = new char[1];
>          this.astralChar = new char[2];
> +        this.endTagExpectation = null;
> +        this.endTagExpectationAsArray = new char[0];

Should have initialized this to null instead.

@@ +608,4 @@
>          this.bmpChar = new char[1];
>          this.astralChar = new char[2];
> +        this.endTagExpectation = null;
> +        this.endTagExpectationAsArray = new char[0];

And here.
I got the following error when transpiling into cpp then compiling central though, so something will have to be changed there I guess?

0:33.44 In file included from /home/jonathan/debugging/mozilla-unified/obj-devedition/parser/html/Unified_cpp_parser_html1.cpp:83:
 0:33.44 /home/jonathan/debugging/mozilla-unified/parser/html/nsHtml5Tokenizer.cpp:150:5: error: no matching constructor for initialization of 'jArray<char16_t, int32_t>' (aka 'jArray<char16_t, int>')
 0:33.44   , endTagExpectationAsArray(nullptr)
 0:33.44     ^                        ~~~~~~~
 0:33.45 /home/jonathan/debugging/mozilla-unified/parser/html/jArray.h:51:8: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'nullptr_t' to 'const jArray<char16_t, int>' for 1st argument
 0:33.45 struct jArray
 0:33.45        ^
 0:33.45 /home/jonathan/debugging/mozilla-unified/parser/html/jArray.h:51:8: note: candidate constructor (the implicit move constructor) not viable: no known conversion from 'nullptr_t' to 'jArray<char16_t, int>' for 1st argument
 0:33.45 /home/jonathan/debugging/mozilla-unified/parser/html/jArray.h:51:8: note: candidate constructor (the implicit default constructor) not viable: requires 0 arguments, but 1 was provided
 0:35.04 1 error generated.

Do we need to modify how the transpiling happens for jArray's?
Flags: needinfo?(hsivonen)
(In reply to Jonathan Kingston [:jkt] from comment #22)
> Do we need to modify how the transpiling happens for jArray's?

AFAICT, editing jArray.h should be enough. Specifically, adding a constructor that takes decltype(nullptr) (maybe nullptr_t works now that we no longer support ancient Android SDK compilers) and sets arr to nullptr and length to 0 to struct jArray.
Flags: needinfo?(hsivonen)
Posted patch bug-1466449-parser-null.patch (obsolete) — Splinter Review
Posted patch bug-1466449-central.patch (obsolete) — Splinter Review
Attachment #9018993 - Attachment is obsolete: true
Comment on attachment 9022897 [details] [diff] [review]
bug-1466449-parser-null.patch

The agreed on change that adds to the previous: bug-1466449-parser.patch patch.
Attachment #9022897 - Flags: review?(hsivonen)
Comment on attachment 9022898 [details] [diff] [review]
bug-1466449-support-nullptr.patch

The change you mentioned to make for central to support nullptr_t type jArray's.
Attachment #9022898 - Flags: review?(hsivonen)
Comment on attachment 9022900 [details] [diff] [review]
bug-1466449-central.patch

The updated patch with the nullptr changes that is instead of the previously landed central patch: https://bug1466449.bmoattachments.org/attachment.cgi?id=9018993
Attachment #9022900 - Flags: review?(hsivonen)
Comment on attachment 9022897 [details] [diff] [review]
bug-1466449-parser-null.patch

Review of attachment 9022897 [details] [diff] [review]:
-----------------------------------------------------------------

This won't compile as Java. I meant making jArray deal with
this.endTagExpectationAsArray = null;
Attachment #9022897 - Flags: review?(hsivonen) → review-
Attachment #9023311 - Flags: review?(bugs)
(In reply to Henri Sivonen (:hsivonen) from comment #31)
> this.endTagExpectationAsArray = null;

Doing this should work if jArray.h is patched with the changes I attached.
I also pushed a change to https://hg.mozilla.org/projects/htmlparser/ to make the code compile again as Java.
Attachment #9023311 - Flags: review?(bugs) → review+
Comment on attachment 9022898 [details] [diff] [review]
bug-1466449-support-nullptr.patch

Review of attachment 9022898 [details] [diff] [review]:
-----------------------------------------------------------------

Let's go with the patch I attached for this.
Attachment #9022898 - Flags: review?(hsivonen) → review-
Comment on attachment 9022900 [details] [diff] [review]
bug-1466449-central.patch

Review of attachment 9022900 [details] [diff] [review]:
-----------------------------------------------------------------

This r- follows from the two others.
Attachment #9022900 - Flags: review?(hsivonen) → review-
Using this.endTagExpectationAsArray = null; (twice) on top of what's now on https://hg.mozilla.org/projects/htmlparser/ , landing the patch I attached and smaug r+ed and regenerating the parser should work. Ahead-of-time r+ for doing that.
Attachment #9022898 - Attachment is obsolete: true
Attachment #9022897 - Attachment is obsolete: true
Posted patch bug-1466449-central.patch (obsolete) — Splinter Review
:hsivonen pre approved this change as it's just code generated https://bugzilla.mozilla.org/show_bug.cgi?id=1466449#c37
Attachment #9022900 - Attachment is obsolete: true
Attachment #9023914 - Flags: review+
Comment on attachment 9023914 [details] [diff] [review]
bug-1466449-central.patch

Patch doensn't compile
Attachment #9023914 - Flags: review+ → review-
Attachment #9023914 - Attachment is obsolete: true
Attachment #9023916 - Flags: review+
Checkin-needed on the patches for central:
https://bugzilla.mozilla.org/attachment.cgi?id=9023311
https://bugzilla.mozilla.org/attachment.cgi?id=9023916

The others are for the java parser: https://hg.mozilla.org/projects/htmlparser/ only bug-1466449-parser-null.patch needs checking in.
Keywords: checkin-needed
I'll just land them since this is blocking bug 1392185.
Blocks: 1392185
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5227588be64
addendum - Turn jArray from a struct to class and make it have a constructor from nullptr. r=smaug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/01219b0ae60e
Update Tokenizer.java and StackNode.java to initialize properties. r=hsivonen
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d5227588be64
https://hg.mozilla.org/mozilla-central/rev/01219b0ae60e
Status: REOPENED → RESOLVED
Closed: 10 months ago9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
:emilio, thanks for landing the central patches. Are you able to the following into the HtmlParser? https://bug1466449.bmoattachments.org/attachment.cgi?id=9023918
Flags: needinfo?(emilio)
I don't think I have permission to push to htmlparser, no... 301 Henri :)
Flags: needinfo?(emilio) → needinfo?(hsivonen)
Pushed:
https://hg.mozilla.org/projects/htmlparser/rev/cad84a21eddd

Thanks for fixing this!
Flags: needinfo?(hsivonen)
Whiteboard: [adv-main65-]
You need to log in before you can comment on or make changes to this bug.