Closed
Bug 1466449
Opened 6 years ago
Closed 5 years ago
Update HTML parser Tokenizer.java and StackNode.java for bug 1453795
Categories
(Core :: DOM: HTML Parser, enhancement, P3)
Core
DOM: HTML Parser
Tracking
()
RESOLVED
FIXED
mozilla65
Tracking | Status | |
---|---|---|
firefox65 | --- | fixed |
People
(Reporter: hsivonen, Assigned: jkt)
References
Details
(Keywords: csectype-uninitialized, sec-audit, Whiteboard: [adv-main65-])
Attachments
(4 files, 8 obsolete files)
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.
Updated•6 years ago
|
Group: core-security → dom-core-security
Updated•6 years ago
|
Keywords: csectype-uninitialized,
sec-audit
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.
Flags: needinfo?(hsivonen)
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
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.
Flags: needinfo?(hsivonen)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jkt
Assignee | ||
Comment 5•6 years ago
|
||
Attachment #9018867 -
Flags: review?(hsivonen)
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
Attachment #9018868 -
Flags: review?(hsivonen)
Attachment #9018868 -
Flags: review?(bpostelnicu)
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.
Assignee | ||
Comment 8•6 years ago
|
||
Attachment #9018867 -
Attachment is obsolete: true
Attachment #9018867 -
Flags: review?(hsivonen)
Assignee | ||
Updated•6 years ago
|
Attachment #9018869 -
Flags: review?(hsivonen)
Assignee | ||
Comment 9•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Status: NEW → ASSIGNED
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.
Attachment #9018870 -
Flags: review?(hsivonen) → review-
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!
Attachment #9018869 -
Flags: review?(hsivonen) → review+
Assignee | ||
Comment 12•6 years ago
|
||
Removed formatting for android files.
Attachment #9018870 -
Attachment is obsolete: true
Attachment #9018870 -
Flags: review?(bpostelnicu)
Attachment #9018993 -
Flags: review?(bpostelnicu)
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?
Attachment #9018993 -
Flags: review?(bpostelnicu) → review+
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 :)
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
Keywords: checkin-needed
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
Keywords: checkin-needed
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cb24d2d42554
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
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!
Flags: needinfo?(hsivonen)
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
Status: RESOLVED → REOPENED
status-firefox65:
fixed → ---
Flags: needinfo?(jkt)
Resolution: FIXED → ---
Target Milestone: mozilla65 → ---
Updated•6 years ago
|
Flags: needinfo?(hsivonen)
Reporter | ||
Comment 20•6 years ago
|
||
Pushed to the htmlparser repo before I saw the backout: https://hg.mozilla.org/projects/htmlparser/rev/94eac97d452e
Flags: needinfo?(jkt)
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.
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?
Flags: needinfo?(hsivonen)
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.
Flags: needinfo?(hsivonen)
Assignee | ||
Comment 24•5 years ago
|
||
Assignee | ||
Comment 25•5 years ago
|
||
Assignee | ||
Comment 26•5 years ago
|
||
Attachment #9018993 -
Attachment is obsolete: true
Assignee | ||
Comment 27•5 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.
Attachment #9022897 -
Flags: review?(hsivonen)
Assignee | ||
Comment 28•5 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.
Attachment #9022898 -
Flags: review?(hsivonen)
Assignee | ||
Comment 29•5 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
Attachment #9022900 -
Flags: review?(hsivonen)
Assignee | ||
Comment 30•5 years ago
|
||
Try push to central with both patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdbcfc040ab515bc3e58e1bd05d6a47cea10806c
Reporter | ||
Comment 31•5 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;
Attachment #9022897 -
Flags: review?(hsivonen) → review-
Reporter | ||
Comment 32•5 years ago
|
||
Attachment #9023311 -
Flags: review?(bugs)
Reporter | ||
Comment 33•5 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.
Reporter | ||
Comment 34•5 years ago
|
||
I also pushed a change to https://hg.mozilla.org/projects/htmlparser/ to make the code compile again as Java.
Updated•5 years ago
|
Attachment #9023311 -
Flags: review?(bugs) → review+
Reporter | ||
Comment 35•5 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.
Attachment #9022898 -
Flags: review?(hsivonen) → review-
Reporter | ||
Comment 36•5 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.
Attachment #9022900 -
Flags: review?(hsivonen) → review-
Reporter | ||
Comment 37•5 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.
Assignee | ||
Updated•5 years ago
|
Attachment #9022898 -
Attachment is obsolete: true
Assignee | ||
Updated•5 years ago
|
Attachment #9022897 -
Attachment is obsolete: true
Assignee | ||
Comment 38•5 years ago
|
||
: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+
Assignee | ||
Comment 39•5 years ago
|
||
Comment on attachment 9023914 [details] [diff] [review] bug-1466449-central.patch Patch doensn't compile
Attachment #9023914 -
Flags: review+ → review-
Assignee | ||
Comment 40•5 years ago
|
||
Attachment #9023914 -
Attachment is obsolete: true
Attachment #9023916 -
Flags: review+
Assignee | ||
Comment 41•5 years ago
|
||
Attachment #9023918 -
Flags: review+
Assignee | ||
Comment 42•5 years ago
|
||
Assuming the folloing try run works I'll check this in: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f614f113f99441e2fdddc1cd9b5835b8a2d14d10
Assignee | ||
Comment 43•5 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.
Keywords: checkin-needed
Comment 45•5 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
Keywords: checkin-needed
Comment 46•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d5227588be64 https://hg.mozilla.org/mozilla-central/rev/01219b0ae60e
Status: REOPENED → RESOLVED
Closed: 6 years ago → 5 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Assignee | ||
Comment 47•5 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
Flags: needinfo?(emilio)
Comment 48•5 years ago
|
||
I don't think I have permission to push to htmlparser, no... 301 Henri :)
Flags: needinfo?(emilio) → needinfo?(hsivonen)
Reporter | ||
Comment 49•5 years ago
|
||
Pushed: https://hg.mozilla.org/projects/htmlparser/rev/cad84a21eddd Thanks for fixing this!
Flags: needinfo?(hsivonen)
Updated•5 years ago
|
Whiteboard: [adv-main65-]
You need to log in
before you can comment on or make changes to this bug.
Description
•