1497146 - Convert FormData.jsm to C++

Status: RESOLVED FIXED

(Firefox :: Session Restore, enhancement, P1)

Description

[Alphan Chen [:alchen]]

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.

Comment 1

[Mike de Boer [:mikedeboer]]

Actively being worked on, so triaged as P1.

Comment 2

[Alphan Chen [:alchen]]

In this bug, I will add a SessionStoreUtils.webidl file and do the rewriting of FormData.jsm.

Comment 3

[Alphan Chen [:alchen]]

Attachment: Bug 1497146 part 1 - Add SessionStoreUtils.webidl

Let SessionStoreUtils be a WebIDL namespace, rather than a XPCOM service

Comment 4

[Alphan Chen [:alchen]]

Attachment: WIP - Bug 1497146 - Convert FormData.jsm to C++ [collect part]

Comment 5

[Alphan Chen [:alchen]]

Attachment: PATCH for the test

Use C++ implementation(attachment 9025087 [details] [diff] [review]) to do the test.

Comment 6

[Alphan Chen [:alchen]]

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

Comment 7

[Alphan Chen [:alchen]]

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()
}

Comment 8

[Alphan Chen [:alchen]]

Based on comment 6 and comment 7, I am thinking about is there a better data format for collect() and restore().

Comment 9

[Jan Henning [:JanH]]

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

Comment 10

[Alphan Chen [:alchen]]

(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()
> }

Comment 11

[Alphan Chen [:alchen]]

Attachment: Bug 1497146 part 2 - Convert FormData.jsm to C++ [collect() part]

Comment 12

[Alphan Chen [:alchen]]

Attachment: Bug 1497146 part 3 - Convert FormData.jsm to C++ [restore() part]

Comment 13

[Alphan Chen [:alchen]]

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

Comment 14

[Alphan Chen [:alchen]]

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.

Comment 15

[Alphan Chen [:alchen]]

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;
};

Comment 16

[Alphan Chen [:alchen]]

I've updated the patches for review.
By using the dictionary describe in comment 15, we can retire FormData.jsm.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dbea03a5c55e
part 1 - Add SessionStoreUtils.webidl r=nika

Comment 18

[Cosmin Sabou [:CosminS]]

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

Comment 19

[Alphan Chen [:alchen]]

(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

[Jan Henning [:JanH]]

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.

Comment 21

[Alphan Chen [: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.

Comment 22

[Alphan Chen [: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

Comment 23

[Alphan Chen [:alchen]]

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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
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

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/67fa6cd74c6a
https://hg.mozilla.org/mozilla-central/rev/b1a1231573cd

Comment 26

[Kris Maglione [:kmag]]

Was part 3 meant to land?

Comment 27

[Hsin-Yi Tsai [:hsinyi]]

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

Was part 3 meant to land?

I believe that's Alphan's plan.

Comment 28

[Alphan Chen [:alchen]]

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

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a6cc9b15b1e3
part 3 - Convert FormData.jsm to C++ [restore() part] r=peterv,mikedeboer

Comment 30

[Arthur Iakab [arthur_iakab]]

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

Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=223765033&revision=a6cc9b15b1e3f464589488b0f490eadd9bb20f74

Failure log https://treeherder.mozilla.org/logviewer.html#?job_id=223745014&repo=autoland

:alchen can you please take a look?

Comment 31

[Alphan Chen [:alchen]]

I am checking the failure now.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/21ad3aeb636f
part 3 - Convert FormData.jsm to C++ [restore() part] r=peterv,mikedeboer

Comment 33

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/21ad3aeb636f

Comment 34

[Andrei Ciure[:andrei_ciure]]

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

Comment 35

[Alphan Chen [:alchen]]

I am checking the crashes.

Comment 36

[Neha Kochar [:neha]]

Alphan, any update on the crash fixes?

Comment 37

[Alphan Chen [: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.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a66d868f77c4
part 3 - Convert FormData.jsm to C++ [restore() part] r=peterv,mikedeboer

Comment 39

[Cosmin Sabou [:CosminS]]

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