Allocate input type=file related member variables only when needed

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jessica, Assigned: jessica)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
To reduce the size of HTMLInputElement, we're going to allocate member variables for input type=file only when input type is file. See Bug 1345767 for details.
(Assignee)

Comment 1

2 years ago
Created attachment 8859882 [details] [diff] [review]
patch, v1.

Hi baku, could you take a look at this? Thanks.
Attachment #8859882 - Flags: review?(amarchesini)
Comment on attachment 8859882 [details] [diff] [review]
patch, v1.

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

::: dom/html/HTMLInputElement.cpp
@@ +1183,5 @@
>      tmp->mInputData.mState->Unlink();
>    }
>  
> +  if (tmp->mFileData) {
> +    tmp->mFileData->Unlink();

tmp->mFileData = nullptr;
should be enough if you have Unlink() in the DTOR of FileData.

@@ +3117,3 @@
>    } else {
>      ErrorResult rv;
> +    GetDOMFileOrDirectoryPath(mFileData->mFilesOrDirectories[0], mFileData->mFirstFilePath, rv);

80chars max per line.

@@ +5108,5 @@
>    if (aNewType == NS_FORM_INPUT_FILE || oldType == NS_FORM_INPUT_FILE) {
> +    if (aNewType == NS_FORM_INPUT_FILE) {
> +      mFileData.reset(new FileData());
> +    } else {
> +      mFileData->Unlink();

remove this if you have the DTOR with Unlink().

::: dom/html/HTMLInputElement.h
@@ +1534,5 @@
>       */
>      nsTextEditorState*       mState;
>    } mInputData;
>  
> +  struct FileData

What about if you move this struct into the cpp file and here you just do:

struct FileData;
UniquePtr<FileData> mFileData;

@@ +1597,5 @@
> +      NS_IMPL_CYCLE_COLLECTION_UNLINK(mFilesOrDirectories)
> +      NS_IMPL_CYCLE_COLLECTION_UNLINK(mFileList)
> +      NS_IMPL_CYCLE_COLLECTION_UNLINK(mEntries)
> +      ClearGetFilesHelpers();
> +    }

make a DTOR in this struct:

 ~FileData()
 {
    Unlink();
 }
Attachment #8859882 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 3

2 years ago
(In reply to Andrea Marchesini [:baku] from comment #2)
> Comment on attachment 8859882 [details] [diff] [review]
> patch, v1.
> 
> Review of attachment 8859882 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/html/HTMLInputElement.cpp
> @@ +1183,5 @@
> >      tmp->mInputData.mState->Unlink();
> >    }
> >  
> > +  if (tmp->mFileData) {
> > +    tmp->mFileData->Unlink();
> 
> tmp->mFileData = nullptr;
> should be enough if you have Unlink() in the DTOR of FileData.
> 
> @@ +3117,3 @@
> >    } else {
> >      ErrorResult rv;
> > +    GetDOMFileOrDirectoryPath(mFileData->mFilesOrDirectories[0], mFileData->mFirstFilePath, rv);
> 
> 80chars max per line.

Will do.

> 
> @@ +5108,5 @@
> >    if (aNewType == NS_FORM_INPUT_FILE || oldType == NS_FORM_INPUT_FILE) {
> > +    if (aNewType == NS_FORM_INPUT_FILE) {
> > +      mFileData.reset(new FileData());
> > +    } else {
> > +      mFileData->Unlink();
> 
> remove this if you have the DTOR with Unlink().
> 
> ::: dom/html/HTMLInputElement.h
> @@ +1534,5 @@
> >       */
> >      nsTextEditorState*       mState;
> >    } mInputData;
> >  
> > +  struct FileData
> 
> What about if you move this struct into the cpp file and here you just do:
> 
> struct FileData;
> UniquePtr<FileData> mFileData;

Good idea, will do.

> 
> @@ +1597,5 @@
> > +      NS_IMPL_CYCLE_COLLECTION_UNLINK(mFilesOrDirectories)
> > +      NS_IMPL_CYCLE_COLLECTION_UNLINK(mFileList)
> > +      NS_IMPL_CYCLE_COLLECTION_UNLINK(mEntries)
> > +      ClearGetFilesHelpers();
> > +    }
> 
> make a DTOR in this struct:
> 
>  ~FileData()
>  {
>     Unlink();
>  }

It seems that I can't set mFileData to nullptr during cc unlink. HTMLInputElement::SaveState() called from nsGenericHTMLFormElement::UnbindFromTree is still called after unlink. So, I guess I'll just leave that part as is.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c99da48d12b3b156b2894105b28c723fd9e30687
(Assignee)

Comment 4

2 years ago
Created attachment 8861833 [details] [diff] [review]
patch, v2.

Move struct FileData implementation into HTMLInputElement.cpp. Carrying r+.
Attachment #8859882 - Attachment is obsolete: true
Attachment #8861833 - Flags: review+

Comment 6

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd3c0b6491de
Allocate input type=file related member variables only when needed. f=smaug, r=baku
Keywords: checkin-needed

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bd3c0b6491de
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Our Coverity analysis pointed out a possible bug in this code:
http://searchfox.org/mozilla-central/rev/38fdbbcaa4be3a3f5b0d207dc5d53fb687c42d6a/dom/html/HTMLInputElement.cpp#444-447

The if statement is testing mGetFilesNonRecursiveHelper, but it's unlinking mGetFilesRecursiveHelper. I think the if statement should be testing mGetFilesRecursiveHelper.
Flags: needinfo?(jjong)
(Assignee)

Comment 9

2 years ago
(In reply to Bill McCloskey (:billm) from comment #8)
> Our Coverity analysis pointed out a possible bug in this code:
> http://searchfox.org/mozilla-central/rev/
> 38fdbbcaa4be3a3f5b0d207dc5d53fb687c42d6a/dom/html/HTMLInputElement.cpp#444-
> 447
> 
> The if statement is testing mGetFilesNonRecursiveHelper, but it's unlinking
> mGetFilesRecursiveHelper. I think the if statement should be testing
> mGetFilesRecursiveHelper.

Oh, you're right. I'll upload a follow-up patch once I get back to the office, keeping the ni for tracking.
May I know where are the results of our coverity analysis? I didn't know we had this.
(Assignee)

Updated

2 years ago
Blocks: 1361267
(Assignee)

Comment 10

2 years ago
(In reply to Jessica Jong [:jessica] from comment #9)
> (In reply to Bill McCloskey (:billm) from comment #8)
> > Our Coverity analysis pointed out a possible bug in this code:
> > http://searchfox.org/mozilla-central/rev/
> > 38fdbbcaa4be3a3f5b0d207dc5d53fb687c42d6a/dom/html/HTMLInputElement.cpp#444-
> > 447
> > 
> > The if statement is testing mGetFilesNonRecursiveHelper, but it's unlinking
> > mGetFilesRecursiveHelper. I think the if statement should be testing
> > mGetFilesRecursiveHelper.
> 
> Oh, you're right. I'll upload a follow-up patch once I get back to the
> office, keeping the ni for tracking.
> May I know where are the results of our coverity analysis? I didn't know we
> had this.

Filed bug 1361267.
Flags: needinfo?(jjong)
I think you can email dveditz to get access to the Coverity analysis.
(Assignee)

Comment 12

2 years ago
(In reply to Bill McCloskey (:billm) from comment #11)
> I think you can email dveditz to get access to the Coverity analysis.

Got it, thanks for the info.
You need to log in before you can comment on or make changes to this bug.