Closed
Bug 1341327
Opened 7 years ago
Closed 7 years ago
Add assertions to be sure that FileBlobImpls are created on the parent process only
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: baku, Assigned: baku)
Details
Attachments
(1 file, 2 obsolete files)
6.31 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•7 years ago
|
||
Assignee: nobody → amarchesini
Attachment #8839513 -
Flags: review?(bugs)
Comment 2•7 years ago
|
||
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-
Assignee | ||
Comment 3•7 years ago
|
||
> 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.
Assignee | ||
Comment 4•7 years ago
|
||
I removed the assertion in FileCreationHelper.
Attachment #8839513 -
Attachment is obsolete: true
Attachment #8839808 -
Flags: review?(bugs)
Comment 5•7 years ago
|
||
(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 6•7 years ago
|
||
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-
Assignee | ||
Comment 7•7 years ago
|
||
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).
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8839808 -
Attachment is obsolete: true
Attachment #8839920 -
Flags: review?(bugs)
Updated•7 years ago
|
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
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a511e9786231
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•