Closed Bug 1333631 Opened 3 years ago Closed 3 years ago

Fix misuse of nsIFile::GetNativeTarget in dom/filesystem

Categories

(Core :: DOM: Core & HTML, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: emk, Assigned: emk)

References

Details

Attachments

(1 file, 1 obsolete file)

GetFilesHelperBase::AddExploredDirectory is totally screwed. Sometimes it uses the native charset, sometime it uses UTF-8. We should use UTF-8 or UTF-16 consistently because the native charset is lossy on Windows.
Also I consider to ban nsIFile::GetNativeTarget on Windows because it should never be used on Windows.
Comment on attachment 8830692 [details]
Bug 1333631 - Fix misuse of nsIFile::GetNativeTarget in dom/filesystem.

https://reviewboard.mozilla.org/r/107430/#review108586

r+ with comments applied.

::: dom/filesystem/GetFilesHelper.cpp:459
(Diff revision 1)
>      if (NS_WARN_IF(NS_FAILED(rv))) {
>        return rv;
>      }
>    }
>  
> -  mExploredDirectories.PutEntry(path);
> +  mExploredDirectories.PutEntry(NS_ConvertUTF16toUTF8(path));

You can change the type of mExploredDirectories to support nsStrings:

nsTHashtable<nsStringHashKey> mExploredDirectories;

and remove this NS_ConvertUTF16toUTF8
Attachment #8830692 - Flags: review?(amarchesini) → review+
Comment on attachment 8830693 [details]
Bug 1333631 - Disallow nsIFile::GetNativeTarget on Windows.

https://reviewboard.mozilla.org/r/107432/#review108590

Thank you for working on this.  The try push looks mysteriously busted, though. :(

::: xpcom/io/nsIFile.idl:253
(Diff revision 1)
> +    // Do not use this in XP code because it is lossy on some platforms
> +    // including Windows.

I assume the "XP" here means cross-platform?  Maybe it would be better to spell that out explicitly, or say something like "We only provide this API for Unix-like platforms, because it would be lossy on Windows."
Attachment #8830693 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #5)
> Comment on attachment 8830693 [details]
> Bug 1333631 - Disallow nsIFile::GetNativeTarget on Windows.
> 
> https://reviewboard.mozilla.org/r/107432/#review108590
> 
> Thank you for working on this.  The try push looks mysteriously busted,
> though. :(

Ugh, I will have to resolve the build failures before landing :(
Maybe I understand what happens. Because GetNativeTarget is added between other XPIDL attributes or methods, the vtable layout will be different from what the XPIDL parser expects. So it will screw up nsIFile method calls from JavaScript.
Assignee: nobody → VYV03354
Priority: -- → P1
Comment on attachment 8830693 [details]
Bug 1333631 - Disallow nsIFile::GetNativeTarget on Windows.

https://reviewboard.mozilla.org/r/107432/#review108590

I changed xpidl.py to add [deleted] annotation.
Comment on attachment 8830693 [details]
Bug 1333631 - Disallow nsIFile::GetNativeTarget on Windows.

https://reviewboard.mozilla.org/r/107432/#review108590

> I assume the "XP" here means cross-platform?  Maybe it would be better to spell that out explicitly, or say something like "We only provide this API for Unix-like platforms, because it would be lossy on Windows."

Done.
Attachment #8830693 - Flags: review+ → review?(nfroyd)
Comment on attachment 8830693 [details]
Bug 1333631 - Disallow nsIFile::GetNativeTarget on Windows.

https://reviewboard.mozilla.org/r/107432/#review109256

I'm inclined to say that we don't need to add so much complexity to the xpidl parser when we could get by with a slightly more sophisticated version of your first patch:

```
interface nsIFile : nsISupports
{
  ...define methods as normal...

  // comments should be included in here somewhere.
%{C++
#ifndef XP_WIN
  NS_IMETHOD GetNativeTarget(nsACString& aNativeTarget) = 0;
#endif
%}
};
```

Placing the extra virtual method at the end of the definition means that the vtable ordering should not be different from what xpconnect expects it to be.  Can you try that approach instead?
Attachment #8830693 - Flags: review?(nfroyd) → review+
Comment on attachment 8830693 [details]
Bug 1333631 - Disallow nsIFile::GetNativeTarget on Windows.

Sigh, let's edit the review flag to show what's really going on.
Attachment #8830693 - Flags: review+ → review-
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/6531a89cfcfc
Fix misuse of nsIFile;GetNativeTarget in dom/filesystem. r=baku
https://hg.mozilla.org/integration/autoland/rev/c8fe57b085bd
Disallow nsIFile::GetNativeTarget on Windows. r=froydnj
Oops, I mis-landed without reading the comment. How to backout autoland changeset?
Keywords: leave-open
Comment on attachment 8830693 [details]
Bug 1333631 - Disallow nsIFile::GetNativeTarget on Windows.

https://reviewboard.mozilla.org/r/107432/#review109256

It didn't work because nsILocalFileMac inherits nsIFile and nsILocalFileMac methods is placed after nsIFile methods in the vtable.

I'll drop the second part of this patch for now and find a way to prevent future mistake.
Attachment #8830693 - Attachment is obsolete: true
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/28bb04d0338d
Fix misuse of nsIFile::GetNativeTarget in dom/filesystem. r=baku
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/28bb04d0338d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.