Bug 237447 – Stop setting NSURLIsExcludedFromBackupKey attribute for localSorageDirectory in UI process

• Component: Website Storage
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Sihui Liu]

Created attachment 453789 [details]
Patch

[Sihui Liu]

Created attachment 453873 [details]
Patch

[Chris Dumez]

Comment on attachment 453873 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453873&action=review

> Source/WebKit/ChangeLog:8
> +        Let's do that on storage thread in network process at when the directory is used.

Seems we should drop the "at" ?

[Darin Adler]

Comment on attachment 453789 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453789&action=review

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:355
> +        FileSystem::setExcludedFromBackup(localStorageDirectory, true);

Why? I know this policy is not new, but it’s not obvious why the need for this is different on iOS than on, say, macOS. If that’s not a secret, I think we should mention it here. If it is a secret for some reason, that’s OK then to not mention it here.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:357
> +        FileSystem::setExcludedFromBackup(localStorageDirectory, false);

Why? This seems to reverse what the code did before, and it’s not obvious why this needs to be done. After all, most things are not excluded from backups. So why do we need to explicitly do this? Is this because these directories were unintentionally excluded from backups in the past, and we are trying to heal that mistake? If so, that seems like a transitional issue. Not sure we’d want this code forever. But also, why don’t we want this like we have on iOS? If that’s not a secret, I think we should mention it here. If it is a secret for some reason, that’s OK then to not mention it here.

[Sihui Liu]

(In reply to Darin Adler from comment #4)
> Comment on attachment 453789 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453789&action=review
> 
> > Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:355
> > +        FileSystem::setExcludedFromBackup(localStorageDirectory, true);
> 
> Why? I know this policy is not new, but it’s not obvious why the need for
> this is different on iOS than on, say, macOS. If that’s not a secret, I
> think we should mention it here. If it is a secret for some reason, that’s
> OK then to not mention it here.

I don't know if it's a secret since it's not mentioned in the original commit; the policy is added in rdar://29045531.

> 
> > Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:357
> > +        FileSystem::setExcludedFromBackup(localStorageDirectory, false);
> 
> Why? This seems to reverse what the code did before, and it’s not obvious
> why this needs to be done. After all, most things are not excluded from
> backups. So why do we need to explicitly do this? Is this because these
> directories were unintentionally excluded from backups in the past, and we
> are trying to heal that mistake? If so, that seems like a transitional
> issue. Not sure we’d want this code forever. But also, why don’t we want
> this like we have on iOS? If that’s not a secret, I think we should mention
> it here. If it is a secret for some reason, that’s OK then to not mention it
> here.

I made a mistake on this patch, by thinking that I excluded the directory on all platforms (we should only do it on iOS). 

And then I realized that we have not turned off custom directory yet (so m_shouldUseCustomPaths is always true and this code path never hit), so we don't need to unset the attribute. Please check the updated patch for correct solution.

[Darin Adler]

Oh thanks; I didn’t realize I was looking at the wrong patch.

[Darin Adler]

Comment on attachment 453873 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453873&action=review

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:366
> +#if PLATFORM(IOS_FAMILY)
> +    if (!m_resolvedLocalStoragePath.isEmpty())
> +        FileSystem::excludeFromBackup(FileSystem::parentPath(m_customLocalStoragePath));
> +#endif

This sure is crying out for a why comment. Some day someone should add one.

[Darin Adler]

Comment on attachment 453873 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453873&action=review

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:366
>> +#endif
> 
> This sure is crying out for a why comment. Some day someone should add one.

Both why we exclude the directory from backups on iOS and why we do not on macOS.

[Sihui Liu]

(In reply to Darin Adler from comment #8)
> Comment on attachment 453873 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453873&action=review
> 
> >> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:366
> >> +#endif
> > 
> > This sure is crying out for a why comment. Some day someone should add one.
> 
> Both why we exclude the directory from backups on iOS and why we do not on
> macOS.

I can add comment like "We only exclude this directory on iOS due to rdar://29045531".
Do you think it's sufficient?

[Darin Adler]

(In reply to Sihui Liu from comment #9)
> I can add comment like "We only exclude this directory on iOS due to
> rdar://29045531".
> Do you think it's sufficient?

It might be sufficient for people at Apple who can follow that link. But is there a reason we have to use a link to explain and can’t give the explanation instead? And nothing about that comment explains why we do this only for iOS family.

[Sihui Liu]

Created attachment 453902 [details]
Patch for landing

[Sihui Liu]

(In reply to Darin Adler from comment #10)
> (In reply to Sihui Liu from comment #9)
> > I can add comment like "We only exclude this directory on iOS due to
> > rdar://29045531".
> > Do you think it's sufficient?
> 
> It might be sufficient for people at Apple who can follow that link. But is
> there a reason we have to use a link to explain and can’t give the
> explanation instead? And nothing about that comment explains why we do this
> only for iOS family.

Talked to Geoff and Brady about this: we did this to reduce backup traffic to server; macOS does backup on device so it does not need this. I added comment in code.

[EWS]

Committed r290863 (248094@main): <https://commits.webkit.org/248094@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453902 [details].

[Radar WebKit Bug Importer]

<rdar://problem/89850452>