Closed Bug 1299500 Opened 3 years ago Closed 3 years ago

Get rid of DeviceStorage API

Categories

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

50 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 53+ fixed
firefox53 + fixed
firefox54 + fixed
firefox55 + fixed

People

(Reporter: baku, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(13 files, 1 obsolete file)

6.55 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
9.38 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
14.59 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
8.80 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
27.97 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
16.38 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
18.74 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
8.60 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
8.68 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
378.57 KB, patch
ehsan
: review+
billm
: review+
Details | Diff | Splinter Review
58.04 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
8.84 KB, text/plain
Details
378.75 KB, text/plain
Details
No description provided.
Depends on: CVE-2017-5456
Priority: -- → P2
Attachment #8844472 - Flags: review?(ehsan)
Attachment #8844474 - Flags: review?(ehsan)
Attachment #8844481 - Flags: review?(ehsan)
Attachment #8844483 - Flags: review?(ehsan)
Attachment #8844484 - Flags: review?(ehsan)
Attachment #8844485 - Flags: review?(ehsan)
This actor is needed for Entries API. I moved some code here without removing the actor.
Attachment #8844487 - Flags: review?(ehsan)
Attachment #8844486 - Flags: review?(ehsan)
I haven't found a nice way to split this patch.
Attachment #8844489 - Flags: review?(ehsan)
Attachment #8844490 - Flags: review?(ehsan)
Note that I didn't touch dom/system/gonk. This code now is broken, but probably it's already broken: it's not compiled in our treeherder builds and it's not used anymore (am I wrong?).
(In reply to Andrea Marchesini [:baku] from comment #12)
> Note that I didn't touch dom/system/gonk. This code now is broken, but
> probably it's already broken: it's not compiled in our treeherder builds and
> it's not used anymore (am I wrong?).

You're right, dom/system/gonk isn't built.  I think we're not supposed to remove gonk code (yet?)
Attachment #8844471 - Flags: review?(ehsan) → review+
Attachment #8844472 - Flags: review?(ehsan) → review+
Attachment #8844474 - Flags: review?(ehsan) → review+
Attachment #8844481 - Flags: review?(ehsan) → review+
Attachment #8844483 - Flags: review?(ehsan) → review+
Comment on attachment 8844484 [details] [diff] [review]
part 6 - Directory::CreateDirectory

Review of attachment 8844484 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/filesystem/Directory.h
@@ -18,5 @@
> -// Windows header files. Undefine the macro of CreateDirectory to avoid
> -// Directory#CreateDirectory being replaced by Directory#CreateDirectoryW.
> -#ifdef CreateDirectory
> -#undef CreateDirectory
> -#endif

Be careful removing this stuff... Make sure to get Windows try runs.  ;-)
Attachment #8844484 - Flags: review?(ehsan) → review+
Attachment #8844485 - Flags: review?(ehsan) → review+
Attachment #8844486 - Flags: review?(ehsan) → review+
Attachment #8844487 - Flags: review?(ehsan) → review+
Attachment #8844489 - Flags: review?(ehsan) → review+
Blocks: nukeb2g
I'll look at the last part tomorrow.
Attachment #8844490 - Flags: review?(ehsan) → review+
Attachment #8844489 - Flags: review?(wmccloskey)
Attachment #8844489 - Flags: review?(wmccloskey) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92606f748fcb
Get rid of DeviceStorage API - part 1 - DeviceStorageAreaChangedEvent, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/41cbf92cfad2
Get rid of DeviceStorage API - part 2 - DeviceStorageChangeEvent, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/07ed303b1e3e
Get rid of DeviceStorage API - part 3 - DeviceStorageAreaListener, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/a35a754a55d8
Get rid of DeviceStorage API - part 4 - Navigator, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8670bee71a5
Get rid of DeviceStorage API - part 5 - Directory::CreateFile, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6e90b7c442f
Get rid of DeviceStorage API - part 6 - Directory::CreateDirectory, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/f28b200fd93d
Get rid of DeviceStorage API - part 7 - Directory::Remove, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4c12243de18
Get rid of DeviceStorage API - part 8 - Directory::Get, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/3364716e5765
Get rid of DeviceStorage API - part 9 - Directory::GetRoot, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d0ea5812180
Get rid of DeviceStorage API - part 10 - DeviceStorage, r=ehsan, r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/c539b2e9f9c9
Get rid of DeviceStorage API - part 11 - FileSystem, r=ehsan
Bug 1344415 requires this bug to be uplifted.
No longer depends on: CVE-2017-5456
Comment on attachment 8844471 [details] [diff] [review]
part 1 - DeviceStorageAreaChangedEvent

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1332003

See comment https://bugzilla.mozilla.org/show_bug.cgi?id=1344415#c39 for more information about why uplift these patches.
Attachment #8844471 - Flags: approval-mozilla-aurora?
Attachment #8844472 - Flags: approval-mozilla-aurora?
Attachment #8844474 - Flags: approval-mozilla-aurora?
Attachment #8844481 - Flags: approval-mozilla-aurora?
Attachment #8844483 - Flags: approval-mozilla-aurora?
Attachment #8844484 - Flags: approval-mozilla-aurora?
Attachment #8844485 - Flags: approval-mozilla-aurora?
Attachment #8844486 - Flags: approval-mozilla-aurora?
Attachment #8844487 - Flags: approval-mozilla-aurora?
Attachment #8844489 - Flags: approval-mozilla-aurora?
Attachment #8844490 - Flags: approval-mozilla-aurora?
Comment on attachment 8844471 [details] [diff] [review]
part 1 - DeviceStorageAreaChangedEvent

Required fix for bug 1344415. Aurora54+.
Attachment #8844471 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8844472 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8844474 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8844481 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8844483 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8844484 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8844485 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8844486 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8844487 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8844489 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8844490 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
at least part 10 needs rebasing like

warning: conflicts while merging toolkit/components/jsdownloads/src/DownloadIntegration.jsm! (edit, then use 'hg resolve --mark')
warning: conflicts while merging toolkit/components/jsdownloads/test/unit/common_test_Download.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(amarchesini)
Attached file 3364716e5765 for m-a
Flags: needinfo?(amarchesini)
Attached file 6d0ea5812180 for m-a (obsolete) —
need l10n approval too 

remote: ************************** ERROR ****************************
remote: 
remote: * File used for localization (mobile/android/locales/en-US/chrome/browser.properties) altered in this changeset *
remote: 
remote: This repository is string frozen. Please request explicit permission from
remote: release managers to break string freeze in your bug.
remote: If you have that explicit permission, denote that by including in
remote: your commit message l10n=...
remote: *************************************************************
Flags: needinfo?(francesco.lodolo)
seems thats from part 10
Can we get a part 10 without the string changes only for aurora? Literally same patch, without the changes to mobile/android/locales/en-US/chrome/browser.properties

Patch is just removing string, but it's going to create a chain effect on tools and a lot of noise mid-cycle. If we can avoid that, it would be great.
Flags: needinfo?(francesco.lodolo) → needinfo?(amarchesini)
Attached file 6d0ea5812180 for m-a
Attachment #8850904 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Required fix for bug 1344415. Aurora54+. -- baku, gchang since this other bug has also more uplift approvals for esr52 and beta we need this one here too right ? Baku if so, can fill out the requests, thanks :)
Flags: needinfo?(gchang)
Flags: needinfo?(amarchesini)
Flags: needinfo?(amarchesini)
Attachment #8844471 - Flags: approval-mozilla-esr52?
Attachment #8844471 - Flags: approval-mozilla-beta?
Attachment #8844472 - Flags: approval-mozilla-esr52?
Attachment #8844472 - Flags: approval-mozilla-beta?
Attachment #8844474 - Flags: approval-mozilla-esr52?
Attachment #8844474 - Flags: approval-mozilla-beta?
Attachment #8844481 - Flags: approval-mozilla-esr52?
Attachment #8844481 - Flags: approval-mozilla-beta?
Attachment #8844483 - Flags: approval-mozilla-esr52?
Attachment #8844483 - Flags: approval-mozilla-beta?
Attachment #8844484 - Flags: approval-mozilla-esr52?
Attachment #8844484 - Flags: approval-mozilla-beta?
Attachment #8844485 - Flags: approval-mozilla-esr52?
Attachment #8844485 - Flags: approval-mozilla-beta?
Attachment #8844486 - Flags: approval-mozilla-esr52?
Attachment #8844486 - Flags: approval-mozilla-beta?
Attachment #8844487 - Flags: approval-mozilla-esr52?
Attachment #8844487 - Flags: approval-mozilla-beta?
Attachment #8844489 - Flags: approval-mozilla-esr52?
Attachment #8844489 - Flags: approval-mozilla-beta?
Comment on attachment 8844490 [details] [diff] [review]
part 11 - FileSystem

[Approval Request Comment]
User impact if declined: These patches are needed for a security fix: Bug 1344415
Fix Landed on Version: aurora and nightly.
Risk to taking this patch (and alternatives if risky): not too much. This API is not exposed to content but just to b2g/connected devices.
String or UUID changes made by this patch:  none


Approval Request Comment
[Feature/Bug causing the regression]: Bug 1332003
[User impact if declined]: This is needed as dependence. No impact to users
[Is this code covered by automated tests?]: it removes code.
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: n/a 
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: none
[Why is the change risky/not risky?]: it removes an already-disabled API.
[String changes made/needed]: none
Attachment #8844490 - Flags: approval-mozilla-esr52?
Attachment #8844490 - Flags: approval-mozilla-beta?
Comment on attachment 8844490 [details] [diff] [review]
part 11 - FileSystem

If this was only used by b2g, should be ok. A bit scary to take this for beta though.
Attachment #8844490 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8844471 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8844472 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8844474 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8844481 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8844483 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8844484 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8844485 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8844486 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8844487 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8844489 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8844471 [details] [diff] [review]
part 1 - DeviceStorageAreaChangedEvent

remove b2g-only api, needed for bug 1344415's fix, esr52+
Attachment #8844471 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Attachment #8844472 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Attachment #8844474 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Attachment #8844481 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Attachment #8844483 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Attachment #8844484 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Attachment #8844485 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Attachment #8844486 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Attachment #8844487 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Attachment #8844489 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Attachment #8844490 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
With the patch in esr52 I am having problems compiling SeaMonkey but I think Thunderbird and Firefox are also affected. Just didn't try yet. I am getting C2061 errors in various comm-esr52 and mozilla-esr52 files which include FileSystemDirectoryReader.h. Always the same error. comm-beta compiles fine so I think the patch needs adjustment.

+++ snip +++

5 -we4553 -GR-  -O2 -Oy   -Fdgenerated.pdb  d:/seabuild/release/comm-esr52-15/obj-x86_64-pc-mingw32/dom/bindings/UnifiedBindings5.cpp
UnifiedBindings5.cpp
d:/seabuild/release/comm-esr52-15/obj-x86_64-pc-mingw32/dist/include\mozilla/dom/FileSystemDirectoryReader.h(31): error C2061: syntax error: identifier 'FileSystemDirectoryEntry'
d:/seabuild/release/comm-esr52-15/obj-x86_64-pc-mingw32/dist/include\mozilla/dom/FileSystemDirectoryReader.h(53): error C2065: 'FileSystemDirectoryEntry': undeclared identifier
d:/seabuild/release/comm-esr52-15/obj-x86_64-pc-mingw32/dist/include\mozilla/dom/FileSystemDirectoryReader.h(53): error C2923: 'RefPtr': 'FileSystemDirectoryEntry' is not a valid template type argument for parameter 'T'
d:/seabuild/release/comm-esr52-15/obj-x86_64-pc-mingw32/dist/include\mozilla/dom/FileSystemDirectoryReader.h(38): error C2678: binary '->': no operator found which takes a left-hand operand of type 'const RefPtr' (or there is no acceptable conversion)
d:\seabuild\release\comm-esr52-15\obj-x86_64-pc-mingw32\dist\include\mozilla/RefPtr.h(316): note: could be 'T *RefPtr<T>::operator ->(void) const'
d:/seabuild/release/comm-esr52-15/obj-x86_64-pc-mingw32/dist/include\mozilla/dom/FileSystemDirectoryReader.h(38): note: while trying to match the argument list '(const RefPtr)'
d:/seabuild/release/comm-esr52-15/obj-x86_64-pc-mingw32/dist/include\mozilla/dom/FileSystemDirectoryReader.h(38): error C2039: 'GetParentObject': is not a member of 'RefPtr'
d:\seabuild\release\comm-esr52-15\obj-x86_64-pc-mingw32\dist\include\mozilla/RefPtr.h(48): note: see declaration of 'RefPtr'
(In reply to Frank-Rainer Grahl from comment #35)
> With the patch in esr52 I am having problems compiling SeaMonkey but I think
> Thunderbird and Firefox are also affected. Just didn't try yet. I am getting
> C2061 errors in various comm-esr52 and mozilla-esr52 files which include
> FileSystemDirectoryReader.h. Always the same error. comm-beta compiles fine
> so I think the patch needs adjustment.
> 
> +++ snip +++
> 
> 5 -we4553 -GR-  -O2 -Oy   -Fdgenerated.pdb 
> d:/seabuild/release/comm-esr52-15/obj-x86_64-pc-mingw32/dom/bindings/
> UnifiedBindings5.cpp
> UnifiedBindings5.cpp
> d:/seabuild/release/comm-esr52-15/obj-x86_64-pc-mingw32/dist/include\mozilla/
> dom/FileSystemDirectoryReader.h(31): error C2061: syntax error: identifier
> 'FileSystemDirectoryEntry'
> d:/seabuild/release/comm-esr52-15/obj-x86_64-pc-mingw32/dist/include\mozilla/
> dom/FileSystemDirectoryReader.h(53): error C2065:
> 'FileSystemDirectoryEntry': undeclared identifier
> d:/seabuild/release/comm-esr52-15/obj-x86_64-pc-mingw32/dist/include\mozilla/
> dom/FileSystemDirectoryReader.h(53): error C2923: 'RefPtr':
> 'FileSystemDirectoryEntry' is not a valid template type argument for
> parameter 'T'
> d:/seabuild/release/comm-esr52-15/obj-x86_64-pc-mingw32/dist/include\mozilla/
> dom/FileSystemDirectoryReader.h(38): error C2678: binary '->': no operator
> found which takes a left-hand operand of type 'const RefPtr' (or there is no
> acceptable conversion)
> d:\seabuild\release\comm-esr52-15\obj-x86_64-pc-mingw32\dist\include\mozilla/
> RefPtr.h(316): note: could be 'T *RefPtr<T>::operator ->(void) const'
> d:/seabuild/release/comm-esr52-15/obj-x86_64-pc-mingw32/dist/include\mozilla/
> dom/FileSystemDirectoryReader.h(38): note: while trying to match the
> argument list '(const RefPtr)'
> d:/seabuild/release/comm-esr52-15/obj-x86_64-pc-mingw32/dist/include\mozilla/
> dom/FileSystemDirectoryReader.h(38): error C2039: 'GetParentObject': is not
> a member of 'RefPtr'
> d:\seabuild\release\comm-esr52-15\obj-x86_64-pc-mingw32\dist\include\mozilla/
> RefPtr.h(48): note: see declaration of 'RefPtr'

I opened https://bugzilla.mozilla.org/show_bug.cgi?id=1353765 for this
Setting qe-verify- based on Andrea's assessment on manual testing needs (see Comment 30).
Flags: qe-verify-
No longer depends on: 1360903
No longer blocks: 1353765
Depends on: 1353765
I've checked MDN, and all of the related API features either haven't been documented at all, or have had their pages moved to the B2G OS archive.

I've added a note to the Fx55 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/55#Removals_from_the_web_platform
No longer blocks: 1369194
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.