Closed Bug 1497146 Opened Last year Closed 10 months ago

Convert FormData.jsm to C++

Categories

(Firefox :: Session Restore, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 66
Tracking Status
firefox66 --- fixed

People

(Reporter: alchen, Assigned: alchen)

References

(Blocks 1 open bug)

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: nobody → alchen
Status: NEW → ASSIGNED
Blocks: 1474130
Actively being worked on, so triaged as P1.
Priority: -- → P1
In this bug, I will add a SessionStoreUtils.webidl file and do the rewriting of FormData.jsm.
Let SessionStoreUtils be a WebIDL namespace, rather than a XPCOM service
Attached patch PATCH for the test (obsolete) — Splinter Review
Use C++ implementation(attachment 9025087 [details] [diff] [review]) to do the test.
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
    ...
}
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()
}
Based on comment 6 and comment 7, I am thinking about is there a better data format for collect() and restore().
(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.
(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()
> }
Attachment #9025087 - Attachment is obsolete: true
Attachment #9025088 - Attachment is obsolete: true
(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.
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.
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;
};
I've updated the patches for review.
By using the dictionary describe in comment 15, we can retire FormData.jsm.
Pushed by shes050117@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/dbea03a5c55e
part 1 - Add SessionStoreUtils.webidl r=nika
(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.
Flags: needinfo?(alchen)
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.
Flags: needinfo?(alchen)
(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.
Flags: needinfo?(alchen)
(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
Flags: needinfo?(csabou)

I figured a way to add bmsvc in existing try submission.

Flags: needinfo?(csabou)
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
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66

Was part 3 meant to land?

Flags: needinfo?(alchen)
Depends on: 1520944

(In reply to Kris Maglione [:kmag] from comment #26)

Was part 3 meant to land?

I believe that's Alphan's plan.

Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---

Yes, part 3 is still waiting for its second review.

Flags: needinfo?(alchen)
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

I am checking the failure now.

Flags: needinfo?(alchen)
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
Depends on: 1523078
Flags: needinfo?(alchen)

I am checking the crashes.

Flags: needinfo?(alchen)

Alphan, any update on the crash fixes?

Flags: needinfo?(alchen)

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

Flags: needinfo?(alchen)
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
Status: REOPENED → RESOLVED
Closed: 11 months ago10 months ago
Resolution: --- → FIXED
Keywords: leave-open
Type: defect → enhancement
Depends on: 1553413
No longer depends on: 1553413
Regressions: 1553413
Blocks: 1564412
You need to log in before you can comment on or make changes to this bug.