Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Reporter
	Description • 6 years ago

Bug 1453795 edited the generated files. Need to change the Java files so as to not undo the changes.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 1 • 6 years ago

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.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 2 • 6 years ago

The following files are the only changes I see when I generate the code:

- parser/html/nsHtml5StackNode.cpp
- parser/html/nsHtml5Tokenizer.cpp

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Reporter
	Comment 3 • 6 years ago

(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.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 4 • 6 years ago

No longer security sensitive.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 6 • 6 years ago

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

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 7 • 6 years ago

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.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 9 • 6 years ago

I had to fix the init order for the "-Wreorder" cpp check to be happy.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Reporter
	Comment 10 • 6 years ago

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.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Reporter
	Comment 11 • 6 years ago

Comment on attachment 9018869 [details] [diff] [review]
bug-1466449-parser.patch

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

Thanks!

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 12 • 6 years ago

Removed formatting for android files.

	Andi [:andi]
	Comment 13 • 6 years ago

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?

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 14 • 6 years ago

> 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 :)

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 15 • 6 years ago

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

	Pulsebot
	Comment 16 • 6 years ago

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

	Bogdan Tara[:bogdan_tara | bogdant]
	Comment 17 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/cb24d2d42554

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 18 • 6 years ago

Are you able to check in the parser changes again? I'm not sure if there is a process for doing these patches?

Thanks!

	Andrei Ciure[:aciure]
	Comment 19 • 6 years ago

Backed out changeset cb24d2d42554 (bug 1466449) for Tokenizer leaks

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=207257453&revision=cb24d2d42554fb0f0f01ee5de3187d19e9985de4

failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=207259075&searchStr=linux%2Cx64%2Casan%2Cmochitests%2Cwith%2Ce10s%2Ctest-linux64-asan%2Fopt-mochitest-e10s-5%2Cm-e10s%285%29

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/d088f3b8a8058e34f43d9aae909c027b41dd2f11

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Reporter
	Comment 20 • 6 years ago

Pushed to the htmlparser repo before I saw the backout:
https://hg.mozilla.org/projects/htmlparser/rev/94eac97d452e

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Reporter
	Comment 21 • 6 years ago

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.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 22 • 6 years ago

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?

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Reporter
	Comment 23 • 6 years ago

(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.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 27 • 6 years ago

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.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 28 • 6 years ago

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.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 29 • 6 years ago

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

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 30 • 6 years ago

Try push to central with both patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdbcfc040ab515bc3e58e1bd05d6a47cea10806c

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Reporter
	Comment 31 • 6 years ago

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;

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Reporter
	Comment 33 • 6 years ago

(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.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Reporter
	Comment 34 • 6 years ago

I also pushed a change to https://hg.mozilla.org/projects/htmlparser/ to make the code compile again as Java.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Reporter
	Comment 35 • 6 years ago

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.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Reporter
	Comment 36 • 6 years ago

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.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Reporter
	Comment 37 • 6 years ago

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.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 38 • 6 years ago

:hsivonen pre approved this change as it's just code generated https://bugzilla.mozilla.org/show_bug.cgi?id=1466449#c37

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 39 • 6 years ago

Comment on attachment 9023914 [details] [diff] [review]
bug-1466449-central.patch

Patch doensn't compile

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 42 • 6 years ago

Assuming the folloing try run works I'll check this in: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f614f113f99441e2fdddc1cd9b5835b8a2d14d10

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 43 • 6 years ago

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 44 • 6 years ago

I'll just land them since this is blocking bug 1392185.

	Pulsebot
	Comment 45 • 6 years ago

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

	Stefan Hindli [:stefan_hindli]
	Comment 46 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/d5227588be64
https://hg.mozilla.org/mozilla-central/rev/01219b0ae60e

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 47 • 6 years ago

:emilio, thanks for landing the central patches. Are you able to the following into the HtmlParser? https://bug1466449.bmoattachments.org/attachment.cgi?id=9023918

	Emilio Cobos Álvarez (:emilio)
	Comment 48 • 6 years ago

I don't think I have permission to push to htmlparser, no... 301 Henri :)

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Reporter
	Comment 49 • 6 years ago

Pushed:
https://hg.mozilla.org/projects/htmlparser/rev/cad84a21eddd

Thanks for fixing this!