Closed Bug 1357981 Opened 4 years ago Closed 4 years ago

Allocate input type=file related member variables only when needed

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jessica, Assigned: jessica)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch, v1. (obsolete) — Splinter Review
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+
(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
Attached patch patch, v2.Splinter Review
Move struct FileData implementation into HTMLInputElement.cpp. Carrying r+.
Attachment #8859882 - Attachment is obsolete: true
Attachment #8861833 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/bd3c0b6491de
Status: NEW → RESOLVED
Closed: 4 years ago
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)
(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.
Blocks: 1361267
(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.
(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.