Convert FormData.jsm to C++
Categories
(Firefox :: Session Restore, enhancement, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: alchen, Assigned: alchen)
References
Details
Attachments
(3 files, 2 obsolete files)
Before doing bug 1474130, I would like to rewrite the js modules which are used in ContentSessionStore.jsm. In this bug, I will focus on FormData.jsm.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 2•6 years ago
|
||
In this bug, I will add a SessionStoreUtils.webidl file and do the rewriting of FormData.jsm.
Assignee | ||
Comment 3•6 years ago
|
||
Let SessionStoreUtils be a WebIDL namespace, rather than a XPCOM service
Assignee | ||
Comment 4•6 years ago
|
||
Assignee | ||
Comment 5•6 years ago
|
||
Use C++ implementation(attachment 9025087 [details] [diff] [review]) to do the test.
Assignee | ||
Comment 6•6 years ago
|
||
Update current status. In attachment 9025087 [details] [diff] [review], I added a dictionary for the data needed to be collected. dictionary SSFormDataDict { object id; object xpath; DOMString innerHTML; ByteString url; }; The value of id or xpath is an object with the changed property name and property value. Property name is always a string type. Property value has different types. (string, object, array, bool) The number of property is depended on the number of elements that needed to be saved. For example, { "txt":"browser_formdata_0.2532067875705051", -----------> string "reverse_thief":{"type":"file","fileList":["/home/user/secret2"]}, ----> object "bystander":{"type":"file","fileList":["/tmp/466937_test-3.file"]}, "/xhtml:html/xhtml:body/xhtml:select[2]":["2","4"] -----------> array "/xhtml:input[@name='check']":false -----------> bool ... }
Assignee | ||
Comment 7•6 years ago
|
||
However, there are two failures and one modification in attachment 9025088 [details] [diff] [review] to pass all the testcase. 1. In .js, the order of property "xpath" is always after property "url". So I changed the order of data in "browser_formdata_xpath.js". I would find a better way to pass this test. 2. I cannot parse the object sucessfully in two cases. In browser_485482.js, there is one special character. { "/xhtml:html/xhtml:body/xhtml:*[local-name()='bad=name']/xhtml:input":"0.45686206139591934", "/xhtml:html/xhtml:body/xhtml:*[local-name()='worse=name']/xhtml:*[local-name()=concatconcat('l0c@l+na~e"',"'",'�')]/xhtml:input[@name='check']":true } In browser_formdata_xpath.js, there is "\n" in one property value. { "//textarea[2]": "Some text... " + Math.random(), "//textarea[3]": "Some more text\n" + new Date() }
Assignee | ||
Comment 8•6 years ago
|
||
Based on comment 6 and comment 7, I am thinking about is there a better data format for collect() and restore().
Comment 9•6 years ago
|
||
(In reply to Alphan Chen [:alchen] from comment #8) > Based on comment 6 and comment 7, I am thinking about is there a better data > format for collect() and restore(). As always, don't forget about backwards-compatibility with existing session store data, though.
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to Alphan Chen [:alchen] from comment #8) > Based on comment 6 and comment 7, I am thinking about is there a better data > format for collect() and restore(). I don't come out a no good answer for the data format. > > 2. I cannot parse the object sucessfully in two cases. I fixed these two failures today. Will update the patch and ask a feedback for current implementation. > > In browser_485482.js, there is one special character. > { > > "/xhtml:html/xhtml:body/xhtml:*[local-name()='bad=name']/xhtml:input":"0. > 45686206139591934", > > "/xhtml:html/xhtml:body/xhtml:*[local-name()='worse=name']/xhtml:*[local- > name()=concatconcat('l0c@l+na~e"',"'",'�')]/xhtml:input[@name='check']":true > } > > In browser_formdata_xpath.js, there is "\n" in one property value. > { > "//textarea[2]": "Some text... " + Math.random(), > "//textarea[3]": "Some more text\n" + new Date() > }
Assignee | ||
Comment 11•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 12•6 years ago
|
||
Assignee | ||
Comment 13•6 years ago
|
||
(In reply to Alphan Chen [:alchen] from comment #12) > Created attachment 9027001 [details] > Bug 1497146 part 3 - Convert FormData.jsm to C++ [restore() part] Not sure if we should do all the JSON parsing thing in C++. I will wait for the first review before doing it.
Assignee | ||
Comment 14•6 years ago
|
||
Summarize the current status: [Need advise] If there is a way(rather than a string) to represent the data we collect() or the data we want to restore in C++. The data is in JSON format with the changed property name and property value(comment 6). [Current Patch] part 1: Move the utils from XPCOM interface into webidl namespace part 2: Collect function implementation: Construct a JSON object(from a string) in C++ code. part 3: Restore function implementation: Leave the JSON parsing thing in the JSM. So the formdata.jsm is not removed in the current patch. [Discussion] 1. Write a parser for parsing the JS object in C++?! 2. Keep the parsing in JSM. Do the other JSM rewriting(1497147, 1507286, 1507287) first. Then propose a new data type or a new way when doing ContentSessionStore(bug 1474130)/ContentRestore rewriting. In this way, "backwards-compatibility" is an open issue needed to take care of.
Assignee | ||
Comment 15•5 years ago
|
||
After discussion(considering the backwards-compatibility), we use the following format to describe the collectedFormData in webidl for the latest patch(part2). dictionary CollectedFileListValue { DOMString type; sequence<DOMString> fileList; }; dictionary CollectedNonMultipleSelectValue { long selectedIndex; DOMString value; }; // object contains either a CollectedFileListValue or a CollectedNonMultipleSelectValue or Sequence<DOMString> typedef (DOMString or boolean or long or object) CollectedFormDataValue; dictionary CollectedFormData { record<DOMString, CollectedFormDataValue>? id; record<DOMString, CollectedFormDataValue>? xpath; DOMString innerHTML; ByteString url; };
Assignee | ||
Comment 16•5 years ago
|
||
I've updated the patches for review. By using the dictionary describe in comment 15, we can retire FormData.jsm.
Comment 17•5 years ago
|
||
Pushed by shes050117@gmail.com: https://hg.mozilla.org/integration/autoland/rev/dbea03a5c55e part 1 - Add SessionStoreUtils.webidl r=nika
Comment 18•5 years ago
|
||
Backed out for bmsvc bustages on SessionStoreUtils. Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&fromchange=dbea03a5c55e1deb19b8092c0cc13bf33af8e99c&tochange=5088f1dd3230fd48b243e3ae3f56e3a0e4982837&searchStr=bmsvc&selectedJob=220001888 Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=220001888&repo=autoland&lineNumber=28761 Backout link: https://hg.mozilla.org/integration/autoland/rev/5088f1dd3230fd48b243e3ae3f56e3a0e4982837
Assignee | ||
Comment 19•5 years ago
|
||
(In reply to Cosmin Sabou [:CosminS] from comment #18) > Backed out for bmsvc bustages on SessionStoreUtils. > > Push with failures: > https://treeherder.mozilla.org/#/ > jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&fromchange=db > ea03a5c55e1deb19b8092c0cc13bf33af8e99c&tochange=5088f1dd3230fd48b243e3ae3f56e > 3a0e4982837&searchStr=bmsvc&selectedJob=220001888 I will check the bmsvc bustages.
Comment 20•5 years ago
|
||
Seeing as this was backed out anyway, could you hold off a little until I've landed bug 1498812? That would make my life easier in case of uplifting. Thank you.
Assignee | ||
Comment 21•5 years ago
|
||
(In reply to Jan Henning [:JanH] from comment #20) > Seeing as this was backed out anyway, could you hold off a little until I've > landed bug 1498812? That would make my life easier in case of uplifting. > Thank you. Let's see. I think I can wait for a couple of days.
Assignee | ||
Comment 22•5 years ago
|
||
(In reply to Cosmin Sabou [:CosminS] from comment #18) > Backed out for bmsvc bustages on SessionStoreUtils. Could you show me how to trigger a bmsvc build? I tried to build all windows possibility in the following. No bmsvc. No build break. https://treeherder.mozilla.org/#/jobs?repo=try&revision=65a7611b7dffb673957e7e77c0660a841bffbbb4
Assignee | ||
Comment 23•5 years ago
|
||
I figured a way to add bmsvc in existing try submission.
Comment 24•5 years ago
|
||
Pushed by shes050117@gmail.com: https://hg.mozilla.org/integration/autoland/rev/67fa6cd74c6a part 1 - Add SessionStoreUtils.webidl r=nika https://hg.mozilla.org/integration/autoland/rev/b1a1231573cd part 2 - Convert FormData.jsm to C++ [collect() part] r=nika,peterv,mikedeboer
Comment 25•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/67fa6cd74c6a
https://hg.mozilla.org/mozilla-central/rev/b1a1231573cd
Comment 27•5 years ago
|
||
(In reply to Kris Maglione [:kmag] from comment #26)
Was part 3 meant to land?
I believe that's Alphan's plan.
Assignee | ||
Comment 28•5 years ago
|
||
Yes, part 3 is still waiting for its second review.
Updated•5 years ago
|
Comment 29•5 years ago
|
||
Pushed by shes050117@gmail.com: https://hg.mozilla.org/integration/autoland/rev/a6cc9b15b1e3 part 3 - Convert FormData.jsm to C++ [restore() part] r=peterv,mikedeboer
Comment 30•5 years ago
|
||
Backed out changeset a6cc9b15b1e3 (bug 1497146)for hazard build bustage on sessionstore/SessionStoreUtils.cpp CLOSED TREE
Backout revision https://hg.mozilla.org/integration/autoland/rev/930600ca52c6c87b8acf2f73cf7cad4bfde654bb
Failure log https://treeherder.mozilla.org/logviewer.html#?job_id=223745014&repo=autoland
:alchen can you please take a look?
Comment 32•5 years ago
|
||
Pushed by shes050117@gmail.com: https://hg.mozilla.org/integration/autoland/rev/21ad3aeb636f part 3 - Convert FormData.jsm to C++ [restore() part] r=peterv,mikedeboer
Comment 33•5 years ago
|
||
bugherder |
Comment 34•5 years ago
|
||
Backed out 2 changesets (bug 1507286, bug 1497146) for causing multiple crashes in nsFocusManager::GetRedirectedFocus
push that caused the backout:
https://hg.mozilla.org/mozilla-central/rev/21ad3aeb636f
https://hg.mozilla.org/mozilla-central/rev/0509a9edc58a
backout: https://hg.mozilla.org/mozilla-central/rev/b08b9f22ad06e55aa83e9c85c74db82c50552094
Assignee | ||
Comment 37•5 years ago
|
||
(In reply to Neha Kochar [:neha] from comment #36)
Alphan, any update on the crash fixes?
After the investigation, we found the reason for this crash.
The crash happens when we pass a nullptr to "GetRedirectedFocus()".
Since there is no null check in "GetRedirectedFocus()", it crashes.
The scenario is like this:
There is an element called "BTextElement" with string value in the session restore data.
When restoring the page, we try to set the original value to "BTextElement".
However, since the page changed, we cannot get "BTextElement" anymore.
In original patch, we will get nullptr and pass it to "GetRedirectedFocus()".
The revised patch is running try now.
Comment 38•5 years ago
|
||
Pushed by jdai@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a66d868f77c4 part 3 - Convert FormData.jsm to C++ [restore() part] r=peterv,mikedeboer
Comment 39•5 years ago
|
||
bugherder |
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Description
•