Currently checking mimeTypes.rdf causes content-type lookups to cross xpconnect which significantly slows down certain file operations. It's also one of the last few remaining uses of rdf.

can you post a profile of this? what exactly is happening during startup to cause this to show up? most of the consumers of the handler service are js from what i can tell. maybe it's possible to improve the performance of privileged js <-> privileged js xpconnect traversals?

There is a project for content handling flow and settings enhancement. (bug 1270405) We would like to replace mimeType.rdf in this project. Currently, I think indexedDB and sqlite are two possible ways for this replacement.

After preliminary study, I will use indexedDB to do the replacement. bug 332690 is a bug related to current mimeType.rdf design. I would consider the problem mentioned in this bug and try to avoid it in our new design. Will provide a diagram/detailed note later.

While indexedDB is surely a good storage, I'd like you to also compare with using a compressed json for this. The choice can likely be driven by some considerations about the data: 1. is the data too "big" for a text store like json 2. would it be very memory consuming and expensive to retain this data in memory, so that it's suggested to use an on-disk store that allows to query on-the-fly every time? How much memory would it take? do we already keep that info in memory somewhere else for quick lookups? 3. Do we need to do complex lookups of the data, or just simple Map kind queries (has key, get value, ...) I'm asking mostly cause while a database is very flexible, it also has I/O downsides and can end up over-engineering a maybe simpler problem. I don't have the answers, cause I didn't look at our mimetypes.rdf usage patterns, but I'd like you to try finding those answers for a more informed path forward. Off-hand I see mimetypes.rdf is 32KB, I suspect it wouldn't be much bigger in memory, and we could just pay the price vs increasing our I/O and complexity.

Hi Marco, thanks for your comment. I did the study of using json for this bug. (In reply to Marco Bonardo [::mak] from comment #4) > The choice can likely be driven by some considerations about the data: > 1. is the data too "big" for a text store like json No, json should be fine. > 2. would it be very memory consuming and expensive to retain this data in > memory, so that it's suggested to use an on-disk store that allows to query > on-the-fly every time? How much memory would it take? do we already keep > that info in memory somewhere else for quick lookups? It is not a big data. However, we don't retain this data in memory now. We just read the data from mimeType.rdf when we need. > 3. Do we need to do complex lookups of the data, or just simple Map kind > queries (has key, get value, ...) > I think we don't need a complex query if we design the data structure well. > I'm asking mostly cause while a database is very flexible, it also has I/O > downsides and can end up over-engineering a maybe simpler problem. Good point. Currently, I think json is a better candidate. So I will use json as target first and see if there is anything out of our expectation.

sounds good, thank you for looking into that.

As a side note, Gijs pointed out bug 332690 in a mail thread, that underlines how it would be better to split mimetypes.rdf into 2 separate stores (See the solution proposed at https://bugzilla.mozilla.org/show_bug.cgi?id=332690#c14). While it's not mandatory, now that you are designing a new store, it could make sense to look into that and see how much work it would add on top.

(In reply to Marco Bonardo [::mak] from comment #7) > As a side note, Gijs pointed out bug 332690 in a mail thread, that > underlines how it would be better to split mimetypes.rdf into 2 separate > stores (See the solution proposed at > https://bugzilla.mozilla.org/show_bug.cgi?id=332690#c14). > While it's not mandatory, now that you are designing a new store, it could > make sense to look into that and see how much work it would add on top. Thanks for the information. I will look into the bug and see the proposed solution.

After analyzing the bug 332690, the root cause of the bug is the mime type of the corresponding extension is modified after clicking the link. But as bug 332690 comment 14 mentioned, it is hard to detect and define if the mapping between the MIME types and the extension is correct or not. Even though the two level extension-to-type mapping is implemented, Firefox still needs to support users to manually modify the extension-to-type mapping in some where. Another problem is not all users know what the mime type is, most of users only care about how the extension is handled. We can take this issue into the consideration of new store designing. Two-level mapping might not be the final solution, I think we need to discuss with UX team to come out what we will support to users, then we can have a better solution for the problem.

Upload the proposal of current plan.

Update the document.

Comment on attachment 8764843 [details] Proposal v1.1 Hi Justin, here is the proposal for this bug. Could you give some feedbacks and suggest a reviewer for this task?

As I understand it, we have the option of keeping strict compatibility with the current methods, or alternatively creating a new way of accessing the configuration file. The current API does main-thread I/O to read the configuration file and is synchronous, keeping compatibility would probably still mean doing main-thread I/O. So my first question would be, what are the known consumers of each method of the API you described? Do they require a synchronous API, or could they work with an asynchronous API? We should also consider whether we should separate the configuration of the extension-to-content-type mapping from the configuration of the actions associated to a content type.

(In reply to :Paolo Amadini from comment #13) > The current API does main-thread I/O to read the configuration file and is synchronous, keeping > compatibility would probably still mean doing main-thread I/O. In this bug, I would like to focus on getting rid of mimeType.rdf. The API change could be the next step. > So my first question would be, what are the known consumers of each method > of the API you described? For "enumerate()" and "store" , they are used by: 1. browser/components/feeds/WebContentConverter.js (only store) 2. browser/components/preferences/in-content/applications.js (both enumerate() and store) 3. browser/extensions/pdfjs/content/PdfJs.jsm (only store) 4. toolkit/mozapps/handling/content/dialog.js (only store) For "fillHandlerInfo", "exists", "getTypeFromExtension", they all are used by nsExternalHelperAppService. > Do they require a synchronous API, or could they > work with an asynchronous API? Need more time to dig deeper. > We should also consider whether we should separate the configuration of the > extension-to-content-type mapping from the configuration of the actions > associated to a content type. What is the main reason for doing this? For bug 332690, the mime type of the corresponding extension is modified after clicking the specific link. In the current design, the user cannot modify/correct the mime type after the change. So I think it should be solved from UX. Besides, there are some words about getTypeFromExtension in "nsIHandlerService.idl". Maybe we could modify or use "nsIMIMEService::getTypeFromExtension" to correct the error. https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsIHandlerService.idl#107 * Note: in general, you should use nsIMIMEService::getTypeFromExtension * to get a MIME type from a file extension, as that method checks a variety * of other sources besides just the datastore. Use this only when you want * to specifically get only the mapping available in the datastore.

(In reply to Alphan Chen [:alchen] from comment #14) > > The current API does main-thread I/O to read the configuration file and is synchronous, keeping > > compatibility would probably still mean doing main-thread I/O. > In this bug, I would like to focus on getting rid of mimeType.rdf. > The API change could be the next step. Well, to change the back-end format and keep the API you have to rewrite all the regression tests to use the old API and check the new format. Then, when you change the API in the next bug, you would have to rewrite all the regression tests again to use the new API and check the new back-end format. Getting rid of mimeTypes.rdf isn't a goal by itself; while there is a marginal benefit in getting rid of RDF APIs, the main benefit resides in improving things like main-thread I/O, as well as a chance to reduce unneeded code complexity. Also, don't forget that when you change the back-end format you also have to consider "helperApps.js", the other consumer of the RDF file: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/content/helperApps.js This is a module used by the helper app dialog and by add-ons to update the configuration. If I understand correctly, this is done because the current API doesn't provide a method that can be used in place of the other module. It may be better to think about what the consumers of the module actually need now, instead of rewriting this with the same API. This also includes understanding the needs of add-ons (including our own internally developed add-ons) which access mimeTypes.rdf directly or use HelperApps.js. With the plan of changing the file format and the API in two separate steps, we would break them two times. It might be worth considering if there is a shorter path that causes less breakage. > > So my first question would be, what are the known consumers of each method > > of the API you described? > > For "fillHandlerInfo", "exists", "getTypeFromExtension", they all are used > by nsExternalHelperAppService. Thanks! This may mean that it's the main place where we should look for use cases. > > Do they require a synchronous API, or could they > > work with an asynchronous API? > Need more time to dig deeper. Take all the time required to understand this. It's common knowledge that spending a few more weeks early in studying a project may save months later! > > We should also consider whether we should separate the configuration of the > > extension-to-content-type mapping from the configuration of the actions > > associated to a content type. > What is the main reason for doing this? They may be needed at different times, and if one set of information is large and the other small or often empty, there may be benefits in storing them in separate files. If not, we could use a single file, but a different data layout that does not mix the two together may make more sense and improve performance. For example, if the file extension is often used as a unique key in a lookup, we could use it as the property name on a JSON object. Do you have this information about the shape of the data? > Besides, there are some words about getTypeFromExtension in > "nsIHandlerService.idl". > Maybe we could modify or use "nsIMIMEService::getTypeFromExtension" to > correct the error. If this is the only relevant consumer (it's likely, because nsIMIMEService is implemented by nsExternalHelperAppService.cpp) we're certainly free to change things as needed. There's also a note in nsIHandlerService.idl about the wish to separate access to OS and user application handlers: https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsIHandlerService.idl#25-36 This may be something that could also benefit the planned UX for handlers. Speaking of which, do you know what are the possible methods that are most likely to be needed by the newly designed UX to configure the handler information?

As a historical note, most the original handling API was designed and evolved over time, when we had feature freeze and interface freeze for releases, and used this as a way of supporting embedding Gecko and XULRunner into other applications. This is gone now, so we may find that when we boil things down to individual use cases, things are really, really, really easier than the API design leads us to think.

(In reply to :Paolo Amadini [Away] from comment #15) > (In reply to Alphan Chen [:alchen] from comment #14) > > Getting rid of mimeTypes.rdf isn't a goal by itself; while there is a > marginal benefit in getting rid of RDF APIs, the main benefit resides in > improving things like main-thread I/O, as well as a chance to reduce > unneeded code complexity. > Hmmm, make sense. > Also, don't forget that when you change the back-end format you also have to > consider "helperApps.js", the other consumer of the RDF file: > > https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/ > content/helperApps.js > > This is a module used by the helper app dialog and by add-ons to update the > configuration. If I understand correctly, this is done because the current > API doesn't provide a method that can be used in place of the other module. > It may be better to think about what the consumers of the module actually > need now, instead of rewriting this with the same API. Thanks for your reminding. > > This also includes understanding the needs of add-ons (including our own > internally developed add-ons) which access mimeTypes.rdf directly or use > HelperApps.js. With the plan of changing the file format and the API in two > separate steps, we would break them two times. It might be worth considering > if there is a shorter path that causes less breakage. > Agree. > > > Besides, there are some words about getTypeFromExtension in > > "nsIHandlerService.idl". > > Maybe we could modify or use "nsIMIMEService::getTypeFromExtension" to > > correct the error. > > If this is the only relevant consumer (it's likely, because nsIMIMEService > is implemented by nsExternalHelperAppService.cpp) we're certainly free to > change things as needed. > > There's also a note in nsIHandlerService.idl about the wish to separate > access to OS and user application handlers: > > https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/ > nsIHandlerService.idl#25-36 > > This may be something that could also benefit the planned UX for handlers. > Speaking of which, do you know what are the possible methods that are most > likely to be needed by the newly designed UX to configure the handler > information? I haven't discussed this bug with them. Will check with them in the project meeting next week.

(In reply to Alphan Chen [:alchen] from comment #17) > I haven't discussed this bug with them. > Will check with them in the project meeting next week. UX team will consider this problem in the future but not currently.

(In reply to :Paolo Amadini from comment #15) > Getting rid of mimeTypes.rdf isn't a goal by itself; while there is a > marginal benefit in getting rid of RDF APIs, the main benefit resides in > improving things like main-thread I/O, as well as a chance to reduce > unneeded code complexity. I disagree; the RDF file format and APIs are difficult and confusing to use, poorly designed, and no one is familiar with them. It's a serious barrier to being able to fix/modify the code involved or add new features. That is a worthy goal in and of itself. Nor would I want to see us invest significant effort in improving the current implementation. See also: modernizing our platform and Great or Dead. (Of course the other things you mention are also good, and I'm not making an argument about the benefits relative to each other.)

As Dolske said, it is a worthy goal to just get rid of mimeType.rdf itself. After internal discussion, it is risky to use a new file format with API change. I would like to fix the scope of this bug as RDF replacement only. Bug 1286154 is for the further discussion of API implementation improvement. Hi Dolske, will you be the reviewer of this bug?

I'd suggest Paolo as the person for feedback/reviews on the design and implementation of this. I don't really have the cycles, and his experience with download manager is probably more useful here. But I'm happy to give some specific input if requested. One clarification, though: (In reply to Alphan Chen [:alchen] from comment #20) [...] > I would like to fix the scope of this bug as RDF replacement only. > Bug 1286154 is for the further discussion of API implementation improvement. This makes sense to me, my comment 19 was talking about the actual RDF APIs, for example all the goop in the nsIRDF* interfaces. I'm far less concerned about the nsIHandlerService API... It might benefit from a redesign, but as long as we're not writing new code that uses or exposes RDF, I'll be happy. :) A couple other bugs I'll point out for more context: Bug 559505 removed localstore.rdf, in favor of simple JSON storage. It added an API (nsIXULStore) where none existed previously -- callers were querying the localstore.rdf data directly through RDF APIs. Bug 853549 switched our password storage from a SQL database to JSON. Paolo may be familiar with this bug. ;-) The interesting approach there is that we were constrained to keep the existing non-async API (for compatibility), and so the code basically (1) reads all the JSON into memory to allow synchronous ops, (2) lazily writes to disk after updates, and (3) jumps through some hoops to fall back to synchronous IO if a call comes in before we've done the initial async read of the JSON. #3 was kind of annoying, but wasn't too bad. So, I'd agree that a fresh implementation of the existing nsIHandlerService API would be a good first step, and then we can come back as a followup step to clean up and improve the API. But I also don't think keeping the existing API is a firm requirement -- if you find that it's doing something particularly dumb or hard to implement sanely, feel free to change it. It's just good scope/risk reduction to avoid that unless needed.

Update the proposal with more information and future work.

One thing to keep into account is that we will need to keep some RDF API calls for a few releases until we can get rid of the migration code. At that point, we'll need to remove the use of the RDF API completely. This might be easier to do if, instead of editing the current JS implementation files of nsIHandlerService and add case switches, we kept those files as they are, and added a separate implementation file for nsIHandlerService handling the JSON back-end. Based on the results of the investigation in bug 1287673, we might even find that it's faster to implement something different from nsIHandlerService. Just like Justin noted in comment 21, we shouldn't be constrained by the current API if it makes the progress slower. We'll revise the current breakdown and dependencies based on these investigation results.

When modify thing globally keep in mind that some admins need to add default custom records to mimetypes, for example: <RDF:Seq RDF:about="urn:schemes:root"> <RDF:li RDF:resource="urn:scheme:mailto"/> <RDF:li RDF:resource="urn:scheme:ms-word"/> <RDF:li RDF:resource="urn:scheme:ms-excel"/> <RDF:li RDF:resource="urn:scheme:ms-powerpoint"/> <RDF:li RDF:resource="urn:scheme:webcal"/> </RDF:Seq> ............................. <RDF:Description RDF:about="urn:scheme:handler:ms-powerpoint" NC:useSystemDefault="true" NC:alwaysAsk="false" /> <RDF:Description RDF:about="urn:scheme:ms-powerpoint" NC:value="ms-powerpoint"> <NC:handlerProp RDF:resource="urn:scheme:handler:ms-powerpoint"/> </RDF:Description> ............................. At this moment this is not possible to new user account, because default profile conetent functionality is removed sync 46.0

Summarize the discussion with Paolo: We will start the work from adding a new nsIHandlerService implementation for the JSON backend. It will be a new file with a new CID. The "datastore utils" of JSON backend may not the same as RDF backend. (datastore utils of RDF backend are HasValue, GetValue, SetValue, RemoveValue) For API investigation, Eden will update the result and action item in bug 1286154. Currently, we will keep the existing non-async API. As bug 853539, we do read synchronously and do write asynchronously. For test plan, we will revise current test case (test_handlerService.js) to cover JSON backend implementation. A new test case for basic datastore utils of JSON backend will be added in this work.

Thanks for the summary! Sounds good to me, in particular I think the JSON store (HandlerStore.jsm) and a stub of the new nsIHandlerService implementation (nsHandlerService-json.js) can be developed in parallel if needed. Can you attach a PDF of the presentation you created to this bug for future reference?

Here is the slides we discuss with Paolo on Tuesday. I also add some conclusion we have made in the meeting.

(In reply to Alphan Chen [:alchen] from comment #27) > I also add some conclusion we have made in the meeting. Thanks, also for the updated engineering breakdown! (There's a typo in the bug number at the end of the presentation, should be bug 853549, but no need to upload a new version just to fix this.)

We didn't get rid of ANY code related to mimetypes.rdf yet, as well as we dind't delete leftover mimetypes.rdf files from profiles. This could be done a in few versions after bug 1287658.

fwiw: http://searchfox.org/mozilla-central/search?q=mimetypes.rdf&path= (note the entry in xperf_whitelist.json should probably be removed)

since 59 is the next ESR, I expect we could remove this code in the 60 cycle.

ESR is now 60

there's also a mimeTypes-android.rdf

Should we get rid of leftover mimetypes.rdf in the profile folder in migrateUI?

Comment on attachment 8958266 [details] Bug 474043 - Part 1 - Get rid of the remaining mimeTypes.rdf references. https://reviewboard.mozilla.org/r/227192/#review233174 I also had the idea of working on this bug, thanks for beating me at it :-) I have a few more changes to request, they should be quick but let me know if you have time to do them, or I can also take over. ::: uriloader/exthandler/nsHandlerService-json.js:56 (Diff revision 1) > - // We have to inject new default protocol handlers only if we haven't > - // already done this when migrating data from the RDF back-end. > + // We have to inject new default protocol handlers. > + this._injectDefaultProtocolHandlers(); This should still be called _injectDefaultProtocolHandlersIfNeeded, and I don't think a comment is needed. ::: uriloader/exthandler/nsHandlerService-json.js:137 (Diff revision 1) > - // Necessary because we want to use this instance of the service, > - // but nsIExternalProtocolHandlerService would call the RDF-based based version > - // until we complete the conversion. > + // Necessary because calling that from here would make XPConnect barf > + // when getService tried to re-enter the constructor for this > + // service. As far as I can tell, the comment you restored here is not true anymore, because the constructor of this object is now lightweight. This is a good catch though, maybe you can try to convert this bit to use nsIExternalProtocolHandlerService.getProtocolHandlerInfo, in a separate changeset? Automated test coverage should be good enough to make this refactoring quite easy. ::: uriloader/exthandler/tests/unit/common_test_handlerService.js (Diff revision 1) > -/* > - * Loaded by "test_handlerService_json.js" and "test_handlerService_rdf.js" to > - * check that the nsIHandlerService interface has the same behavior with both > - * the JSON and RDF backends. > - */ In a separate changeset, we can rename this file to test_handlerService_json.js and inline the existing parts from that file... ::: uriloader/exthandler/tests/unit/test_handlerService_json.js:8 (Diff revision 1) > let gHandlerService = gHandlerServiceJSON; > let unloadHandlerStore = unloadHandlerStoreJSON; > let deleteHandlerStore = deleteHandlerStoreJSON; > let copyTestDataToHandlerStore = copyTestDataToHandlerStoreJSON; ...and we can rename the methods and remove this indirection.

(In reply to Marco Bonardo [::mak] from comment #35) > Should we get rid of leftover mimetypes.rdf in the profile folder in > migrateUI? I don't think we did this for other migrations before. While losing file types associations is less important than for example logins, keeping the file still facilitates downgrades, and in general is less code to write, so I'd keep the file around like we did in other cases.

Comment on attachment 8958266 [details] Bug 474043 - Part 1 - Get rid of the remaining mimeTypes.rdf references. https://reviewboard.mozilla.org/r/227192/#review233836 Add part numbers to the commit messages, like "Bug 474043 - Part 1 - ...".

Comment on attachment 8958517 [details] Bug 474043 - Part 2 - Don't reimplement GetProtocolHandlerInfo. https://reviewboard.mozilla.org/r/227440/#review233834

Comment on attachment 8958518 [details] Bug 474043 - Merge common_test_handlerService.js. https://reviewboard.mozilla.org/r/227442/#review233832 Can you do the merge the other way around, keeping common_test_handlerService.js as the ancestor?

Comment on attachment 8958519 [details] Bug 474043 - Part 4 - Cleanup some JSON suffixes. https://reviewboard.mozilla.org/r/227444/#review233838 Thanks!

Comment on attachment 8959118 [details] Bug 474043 - Part 3 - Merge common_test_handlerService.js and test_handlerService_json.js into test_handlerService_store.js. https://reviewboard.mozilla.org/r/227998/#review233870 ::: uriloader/exthandler/tests/unit/xpcshell.ini:5 (Diff revision 1) > [DEFAULT] > head = head.js > run-sequentially = Bug 912235 - Intermittent failures > firefox-appdir = browser > support-files = common_test_handlerService.js This needs to be removed, right? Is the original file deleted?

Comment on attachment 8959119 [details] Bug 474043 - Part 5 - Remove the json suffix from nsHandlerService. https://reviewboard.mozilla.org/r/228000/#review233874 Can you also remove the "ns" prefix? I know there is another component in the same folder that uses it, but we're not supposed to use the prefix anymore, see for example "DownloadLegacy.js".

Comment on attachment 8959118 [details] Bug 474043 - Part 3 - Merge common_test_handlerService.js and test_handlerService_json.js into test_handlerService_store.js. https://reviewboard.mozilla.org/r/227998/#review233888

Comment on attachment 8959119 [details] Bug 474043 - Part 5 - Remove the json suffix from nsHandlerService. https://reviewboard.mozilla.org/r/228000/#review233890

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/50e6b47ce6b6 Part 1 - Get rid of the remaining mimeTypes.rdf references. r=Paolo https://hg.mozilla.org/integration/autoland/rev/e4be9af80f5c Part 2 - Don't reimplement GetProtocolHandlerInfo. r=Paolo https://hg.mozilla.org/integration/autoland/rev/c30b60800fd2 Part 3 - Merge common_test_handlerService.js and test_handlerService_json.js into test_handlerService_store.js. r=Paolo https://hg.mozilla.org/integration/autoland/rev/ace79d388801 Part 4 - Cleanup some JSON suffixes. r=Paolo https://hg.mozilla.org/integration/autoland/rev/ebac7780fed6 Part 5 - Remove the json suffix from nsHandlerService. r=Paolo

https://hg.mozilla.org/mozilla-central/rev/50e6b47ce6b6 https://hg.mozilla.org/mozilla-central/rev/e4be9af80f5c https://hg.mozilla.org/mozilla-central/rev/c30b60800fd2 https://hg.mozilla.org/mozilla-central/rev/ace79d388801 https://hg.mozilla.org/mozilla-central/rev/ebac7780fed6