Closed Bug 1341327 Opened 3 years ago Closed 3 years ago

Add assertions to be sure that FileBlobImpls are created on the parent process only

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Attached patch file_on_parent.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8839513 - Flags: review?(bugs)
Comment on attachment 8839513 [details] [diff] [review]
file_on_parent.patch

Make sure to look at crash stats after landing this patch to see whether we see the assertions to fail.

Why do you remove the main thread assertion from File::CreateFromNsIFile?
As far as I see, that is valid assert. nsIFile shouldn't be used in any worker (not even chrome worker) since it requires XPConnect to work. (nsIFile isn't a .webidl thing). So, please don't remove that assertion

Change to FileCreatorHelper::CreateFile looks weird. We have 
MOZ_ASSERT(NS_IsMainThread()); and then if (!NS_IsMainThread()) {.
Could you explain this.
Couldn't we make the MOZ_ASSERT MOZ_DIAGNOSTIC_ASSERT and not add the 'if'?
Attachment #8839513 - Flags: review?(bugs) → review-
> Make sure to look at crash stats after landing this patch to see whether we
> see the assertions to fail.

I did. It's green on try.

> Why do you remove the main thread assertion from File::CreateFromNsIFile?

nsIFile can be used in any thread. I just wanted to be use that, from a C++ point of view, we have diagnostic assertions. And from JS, we reject the promises.
Attached patch file_on_parent.patch (obsolete) — Splinter Review
I removed the assertion in FileCreationHelper.
Attachment #8839513 - Attachment is obsolete: true
Attachment #8839808 - Flags: review?(bugs)
(In reply to Andrea Marchesini [:baku] from comment #3)
> > Make sure to look at crash stats after landing this patch to see whether we
> > see the assertions to fail.
> 
> I did. It's green on try.

I mean crash-stats
Comment on attachment 8839808 [details] [diff] [review]
file_on_parent.patch

You didn't answer to my question about FileCreatorHelper::CreateFile.
It looks still weird. Why to explicitly allow code to run on non-main-thread when it is guaranteed to always be rejected.
Attachment #8839808 - Flags: review?(bugs) → review-
Well, better this than crashing, am I wrong? We don't have any needs to expose File.createFromNsIFile and File.createFromFileName to workers. And both methods are chrome only (mainly for testing).
Attachment #8839808 - Attachment is obsolete: true
Attachment #8839920 - Flags: review?(bugs)
Attachment #8839920 - Flags: review?(bugs) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a511e9786231
Add assertions to be sure that FileBlobImpls are created on the parent process only, r=smaug
https://hg.mozilla.org/mozilla-central/rev/a511e9786231
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.