Bug 175522 - Only create directory and sandbox extension handle for storage directories if they are set
Summary: Only create directory and sandbox extension handle for storage directories if...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-13 09:12 PDT by Tim Horton
Modified: 2017-08-14 10:57 PDT (History)
6 users (show)

See Also:


Attachments
Patch (3.23 KB, patch)
2017-08-13 09:12 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
With a weird test (11.16 KB, patch)
2017-08-14 01:58 PDT, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2017-08-13 09:12:37 PDT
Only create directory and sandbox extension handle for storage directories if they are set
Comment 1 Tim Horton 2017-08-13 09:12:59 PDT
Created attachment 318008 [details]
Patch
Comment 2 Brady Eidson 2017-08-13 09:20:36 PDT
Comment on attachment 318008 [details]
Patch

The IDB change was causing tests to fail due to that noisy logging.

Are these others causing tests to fail with noisy logging?

If not, can we add some tests that will fail with the noisy logging?

(Because we should test this kind of thing)
Comment 3 Tim Horton 2017-08-13 13:51:59 PDT
Most tests don’t care about logging; seems like it would be pretty weird to add a perf test to test this.

Maybe we can make a API test that temporarily slurps stdout?
Comment 4 Brady Eidson 2017-08-13 14:22:54 PDT
(In reply to Tim Horton from comment #3)
> Most tests don’t care about logging; seems like it would be pretty weird to
> add a perf test to test this.
> 
> Maybe we can make a API test that temporarily slurps stdout?

My goal wasn't necessarily to make tests care about logging so much as make tests track this behavior.
Comment 5 Tim Horton 2017-08-14 01:58:03 PDT
Created attachment 318027 [details]
With a weird test
Comment 6 Tim Horton 2017-08-14 01:59:33 PDT
(In reply to Brady Eidson from comment #4)
> (In reply to Tim Horton from comment #3)
> > Most tests don’t care about logging; seems like it would be pretty weird to
> > add a perf test to test this.
> > 
> > Maybe we can make a API test that temporarily slurps stdout?
> 
> My goal wasn't necessarily to make tests care about logging so much as make
> tests track this behavior.

But, like you yourself said in https://bugs.webkit.org/show_bug.cgi?id=171522, there's not a great way to test this except for the side-effect of the logging. So I wrote a very strange test that makes sure we don't log during initialization. Which might be valuable and interesting, and might just be bizarre. But it does fail before and pass after...
Comment 7 Brady Eidson 2017-08-14 09:06:05 PDT
(In reply to Tim Horton from comment #6)
> (In reply to Brady Eidson from comment #4)
> > (In reply to Tim Horton from comment #3)
> > > Most tests don’t care about logging; seems like it would be pretty weird to
> > > add a perf test to test this.
> > > 
> > > Maybe we can make a API test that temporarily slurps stdout?
> > 
> > My goal wasn't necessarily to make tests care about logging so much as make
> > tests track this behavior.
> 
> But, like you yourself said in
> https://bugs.webkit.org/show_bug.cgi?id=171522, there's not a great way to
> test this except for the side-effect of the logging. So I wrote a very
> strange test that makes sure we don't log during initialization. Which might
> be valuable and interesting, and might just be bizarre. But it does fail
> before and pass after...

Let me rephrase.

My goal was "Can we make a regression test of this that fails before and passes after?"
My observation was "In the other case, for example, the logging fulfilled this goal"
The unspoken part was "The logging is clearly a bizarre way to test, but if it's the only way, so be it"

Really, just want a test. And if your test is bizarre... but works... so be it.
Comment 8 WebKit Commit Bot 2017-08-14 10:56:51 PDT
Comment on attachment 318027 [details]
With a weird test

Clearing flags on attachment: 318027

Committed r220709: <http://trac.webkit.org/changeset/220709>
Comment 9 WebKit Commit Bot 2017-08-14 10:56:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2017-08-14 10:57:13 PDT
<rdar://problem/33878647>