Bug 24704 – DumpRenderTree (OSX version) should be able to run independent instances at the same time.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[David Levin]

Much like WebView.mm does.

Fixing this would allow multiple instances to run in parallel and not conflict (as long as dumpPixels isn't turned on).

[David Levin]

When I said fixing this issue allows running multiple instances, I mean if the DumpRenderTree instances have different names.  

For example, one could create a symbol link and runs that instead to ensure that the settings are isolated from other instances.

[David Levin]

Created attachment 28765 [details]
Proposed fix.

[Mark Rowe (bdash)]

Why is this useful?

[David Levin]

One of the problems mentioned in https://bugs.webkit.org/show_bug.cgi?id=10906 is "- sharing of icon database, cookies and disk cache;"


After these changes I could do this
  ln -s DumpRenderTree DumpRenderTree1
  ln -s DumpRenderTree DumpRenderTree2

Then I can run DumpRenderTree1 and DumpRenderTree2 at the same time without sharing the above items.  (WebKit on OSX already appears to use the app name to separate settings.)

[Mark Rowe (bdash)]

I don't think that this is the appropriate way to address that problem.

[David Levin]

> I don't think that this is the appropriate way to address that problem.
That's a bit terse.  Would you elaborate on what you think is "the appropriate way"? :)

The other idea I had was to pass in some parameter to DumpRenderTree, but it seems like I would want the end result to be the same (separate location for these items).  In addition, I would need to find and change code like WebKitInitializeApplicationCachePathIfNecessary() in WebView.mm also (while the differing app name worked nicely with this coding pattern).

[Mark Rowe (bdash)]

(In reply to comment #6)
> > I don't think that this is the appropriate way to address that problem.
> That's a bit terse.  Would you elaborate on what you think is "the appropriate
> way"? :)

Sorry, I replied to this from my iPhone and gave up on elaborating.  

Some things, such as cookies, are not application-specific.  Since changing the application name can't address all sharing issues, I think it is better to take a cleaner approach.

> The other idea I had was to pass in some parameter to DumpRenderTree, but it
> seems like I would want the end result to be the same (separate location for
> these items).  In addition, I would need to find and change code like
> WebKitInitializeApplicationCachePathIfNecessary() in WebView.mm also (while the
> differing app name worked nicely with this coding pattern).

I think that having DumpRenderTree generate a unique identifier and using that in paths, etc, to avoid conflicts would be preferable.  It may require additional changes, but some of that functionality will be useful to other WebKit clients, and it seems cleaner than relying on how WebKit and other system frameworks happen to generate cache paths, etc.

[David Levin]

Created attachment 28781 [details]
Proposed fix.

[David Levin]

The remaining issue as I understand it with the current patch is that it doesn't help with the CFUrlCache/CFNetwork.  

After much looking, I can't find anyway to do this in code.  However, it does use the app name in forming the location for the cache, so a good way to address that is to use apps with different names (the ln-s technique).

[Mark Rowe (bdash)]

(In reply to comment #9)
> The remaining issue as I understand it with the current patch is that it
> doesn't help with the CFUrlCache/CFNetwork.  
> 
> After much looking, I can't find anyway to do this in code.  However, it does
> use the app name in forming the location for the cache, so a good way to
> address that is to use apps with different names (the ln-s technique).

A better approach would be to disable the disk cache.

[David Levin]

Created attachment 28863 [details]
Proposed fix.

Address previous issues.

This patch still exports WebKitLocalCacheDefaultsKey as opposed to making it a constant.  I was following the pattern of what was done for WebDatabaseDirectoryDefaultsKey in WebKit/mac/Storage/WebDatabaseManagerPrivate.h.

If something different should be done in this case, let me know and I can change it.

[Eric Seidel (no email)]

Comment on attachment 28863 [details]
Proposed fix.

         else
 313             cacheDir = nil;
isn't needed here.  cacheDir is already known to be nil at the start of that (rather short) if block, right?

Otherwise the approach looks fine to me.  I guess Mark started this review and should comment before landing.  But it looks fine IMO.

[Eric Seidel (no email)]

Comment on attachment 28863 [details]
Proposed fix.

It seems saving things to:
~/Library/tmp is silly.  ~/Library/Application Support/DumpRenderTree or ~/Library/Caches/DumpRenderTree would make more sense.

Otherwise this looks fine.  Mark should feel encouraged to comment further, but this looks like a big step forward for our testing.  Even if it's not perfect final layout, it's much better than what we have today (in terms of that it moves us closer to running tests in parallel).

[David Levin]

Created attachment 30055 [details]
Addressed eseidel's concerns.

[Eric Seidel (no email)]

It looks like this will also fix it so that jparent and I can both run layout tests at the same time on the same machine. :(  Currently if one of us has run the other can't until we clear /tmp because the perl script tries to write the same files and gets permission errors.  At least I think that's what happened.

Dave, would you like me to land this, or do you plan to?

[Mark Rowe (bdash)]

(In reply to comment #15)
> It looks like this will also fix it so that jparent and I can both run layout
> tests at the same time on the same machine. :(  Currently if one of us has run
> the other can't until we clear /tmp because the perl script tries to write the
> same files and gets permission errors.  At least I think that's what happened.

Assuming that you're running as different users, that doesn't sound related to this change at all.  The only path that this patch moves from /tmp is a single PDF output file.  What you're more likely hitting is that the default location for test results is inside /tmp, but there's already a command-line flag to run-webkit-tests to redirect that.

[David Levin]

I'm willing to land this.  I didn't yet because I had heard there were reservations about it and I was trying to give folks a chance to voice up about it....

I'll land it tomorrow unless someone speaks up.

[David Levin]

http://trac.webkit.org/changeset/43721