Bug 188424

Created attachment 346803 [details]
Patch

mac-32bit failure here seems to be a fluke? This patch should not change behavior for other platforms.

If tempfile.NamedTemporaryFile(mode='r', delete=Treu) is used in this case, will the temporary file be deleted automatically when GC collects the file object?

Or, what about the idea defining own mktemp like deprecated tempfile.mktemp?

def mktemp(self):
    fd, filename = tempfile.mkstemp()
    os.close(fd)
    return filename

(In reply to Fujii Hironori from comment #3)
> If tempfile.NamedTemporaryFile(mode='r', delete=Treu) is used in this case,
> will the temporary file be deleted automatically when GC collects the file
> object?

I hadn't tried it, but it certainly seems to work! (Tried on Mac and Win alike.)

Created attachment 346815 [details]
Patch

Comment on attachment 346815 [details]
Patch

So weird. I can't understand why this code works.
NamedTemporaryFile() returns file object.
You pass the file objects to generate_supplemental_dependency, not filenames.
Why does this code work?

You can get the filename by 'supplemental_dependency_file.name'.

(In reply to Fujii Hironori from comment #6)
> Comment on attachment 346815 [details]
> Patch
> 
> So weird. I can't understand why this code works.
> NamedTemporaryFile() returns file object.
> You pass the file objects to generate_supplemental_dependency, not filenames.
> Why does this code work?

Apparently it works because "<open file '<fdopen>', mode 'w+b' at 0x10102ff60>" is a valid filename, hahaha.

(In reply to Fujii Hironori from comment #7)
> You can get the filename by 'supplemental_dependency_file.name'.

Thanks.

Created attachment 346820 [details]
Patch

(In reply to Ross Kirsling from comment #8)
> Apparently it works because "<open file '<fdopen>', mode 'w+b' at
> 0x10102ff60>" is a valid filename, hahaha.

...and because WebCore/bindings/scripts/preprocess-idls.pl will happily create the supplemental_dependency_file if it doesn't file already exists (it just won't clean up after itself), I mean.

(In reply to Ross Kirsling from comment #10)
> ...and because WebCore/bindings/scripts/preprocess-idls.pl will happily
> create the supplemental_dependency_file if it doesn't file already exists
> (it just won't clean up after itself), I mean.

Ugh, failure to proofread. "if the file doesn't already exist".

Comment on attachment 346820 [details]
Patch

Clearing flags on attachment: 346820

Committed r234720: <https://trac.webkit.org/changeset/234720>

All reviewed patches have been landed.  Closing bug.

<rdar://problem/43080517>

It failed on the BuildBot.

https://build.webkit.org/builders/WinCairo%2064-bit%20WKL%20Release%20%28Tests%29/builds/954/steps/bindings-generation-tests/logs/stdio

> Couldn't open c:\users\containeradministrator\appdata\local\temp\tmp6paapw: Permission denied
> 
> Failed to generate a supplemental dependency file.

Do you need to open as read mode?
NamedTemporaryFile(mode='r')

(In reply to Fujii Hironori from comment #15)
> It failed on the BuildBot.
> 
> https://build.webkit.org/builders/WinCairo%2064-
> bit%20WKL%20Release%20%28Tests%29/builds/954/steps/bindings-generation-tests/
> logs/stdio
> 
> > Couldn't open c:\users\containeradministrator\appdata\local\temp\tmp6paapw: Permission denied
> > 
> > Failed to generate a supplemental dependency file.
> 
> Do you need to open as read mode?
> NamedTemporaryFile(mode='r')

Shoot, I only tested the last patch on Mac.

We don't even read from the supplemental dependency file from Python, so the mode doesn't matter (though I made sure just now), but as the docs say:
> Whether the name can be used to open the file a second time, while the named temporary file is still open, varies across platforms (it can be so used on Unix; it cannot on Windows NT or later).

So apparently we need delete=False, which means os.remove is unavoidable after all, but NamedTemporaryFile does still free us from needing a separate os.close call.

(In reply to Ross Kirsling from comment #16)
> (In reply to Fujii Hironori from comment #15)
> > It failed on the BuildBot.
> > 
> > https://build.webkit.org/builders/WinCairo%2064-
> > bit%20WKL%20Release%20%28Tests%29/builds/954/steps/bindings-generation-tests/
> > logs/stdio
> > 
> > > Couldn't open c:\users\containeradministrator\appdata\local\temp\tmp6paapw: Permission denied
> > > 
> > > Failed to generate a supplemental dependency file.
> > 
> > Do you need to open as read mode?
> > NamedTemporaryFile(mode='r')
> 
> Shoot, I only tested the last patch on Mac.
> 
> We don't even read from the supplemental dependency file from Python, so the
> mode doesn't matter (though I made sure just now), but as the docs say:
> > Whether the name can be used to open the file a second time, while the named temporary file is still open, varies across platforms (it can be so used on Unix; it cannot on Windows NT or later).
> 
> So apparently we need delete=False, which means os.remove is unavoidable
> after all, but NamedTemporaryFile does still free us from needing a separate
> os.close call.

Oops, nope -- the fact that this works is almost an accident, because we're depending on Perl closing the file for us.

Regarding your earlier suggestion about mktemp, this function was deprecated for legitimate security reasons, so we really can't justify opting into that behavior:
https://docs.python.org/2/library/tempfile.html#tempfile.mktemp

As suggested by the blog post I linked in the ChangeLog (https://www.logilab.org/blogentry/17873), calling os.close before os.remove seems to be the "right thing to do" even on POSIX, so I will reintroduce my initial approach as a fix.

Committed r234726: <https://trac.webkit.org/changeset/234726>