924702 - Rewrite the app:// protocol handler in c++

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[[:fabrice] Fabrice Desré]

This will help app startup time in b2g.

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: wip

This patch lacks a complete DummyChannel implementation, but since its only used in error cases that doesn't prevent testing in normal conditions.

Vivien, can you re-run your timings with this patch?

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 814730 [details] [diff] [review]
wip

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

::: netwerk/build/Makefile.in
@@ +12,5 @@
>    ../mime/$(LIB_PREFIX)nkmime_s.$(LIB_SUFFIX) \
>    ../cache/$(LIB_PREFIX)nkcache_s.$(LIB_SUFFIX) \
>    ../cache2/$(LIB_PREFIX)nkcache2_s.$(LIB_SUFFIX) \
>    ../protocol/about/$(LIB_PREFIX)nkabout_s.$(LIB_SUFFIX) \
> +  ../protocol/app/$(LIB_PREFIX)nkapp_s.$(LIB_SUFFIX) \

For all the things related to the app protocol I think the correct implementation will be with a new flag in configure.in. Then you will have this for free because of the line just below this one. I didn't did it this way because I was trying to go fast and didn't want to trigger a full rebuild.

You will again gain some #ifdef that you can use in nsNetCID.h and nsNetModule.cpp.

::: netwerk/protocol/app/AppProtocolHandler.cpp
@@ +184,5 @@
> +  mozilla::dom::AppInfo appInfo;
> +  nsAutoString key = NS_ConvertUTF8toUTF16(host);
> +
> +  if (!mAppInfoCache.Get(key, &appInfo)) {
> +    nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID);

I think I was secretly hoping to be able to avoid all js access but doing it only once should not hurt that much. I will test the patch in a few... Seems like it works with similar perf than the poc.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Clearing the needinfo since I have already answered.

Comment 4

[[:fabrice] Fabrice Desré]

Attachment: patch

Comment 5

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Comment 6

[Jason Duell]

Comment on attachment 815191 [details] [diff] [review]
patch v2

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

looks good--only one bug and a few nits.  no need for re-review.

::: dom/webidl/AppInfo.webidl
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> +  * This dictionnary holds the parameters sent to the wifi service.

s/nn/n/

::: netwerk/protocol/app/AppProtocolHandler.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

vim modeline too.

@@ +47,5 @@
> +    if (mSuspendCount <= 0) {
> +      return NS_ERROR_UNEXPECTED;
> +    }
> +
> +    if (--mSuspendCount == 0 && mPending) {

I don't think we need the mPending check here.  It's not in any other channel's Resume method.  The --mSuspendcount == 0 check should already ensure we don't dispatch Run twice.

@@ +60,5 @@
> +
> +  NS_IMETHODIMP AsyncOpen(nsIStreamListener* aListener, nsISupports* aContext) {
> +    mListener = aListener;
> +    mContext = aContext;
> +

You need to do 

  mPending = true; 

in AsyncOpen.  From a peek at nsHttpChannel, it looks like the convention is to add to the loadGroup after setting it to true (though the old .js code didn't and that didn't cause any issues.  But let's do it before just in case).

@@ +64,5 @@
> +
> +    nsCOMPtr<nsILoadGroup> loadGroup;
> +    nsresult rv = GetLoadGroup(getter_AddRefs(loadGroup));
> +    if (NS_SUCCEEDED(rv) && loadGroup) {
> +      loadGroup->AddRequest(this, aContext);

no need to use GetLoadGroup.  Just

  if (mLoadGroup) {
    mLoadGroup->AddRequest(...
  }

@@ +67,5 @@
> +    if (NS_SUCCEEDED(rv) && loadGroup) {
> +      loadGroup->AddRequest(this, aContext);
> +    }
> +
> +    if (mSuspendCount > 0) {

The condition in the JS is "if mSuspendCount == 0", and I think that's still right :)

@@ +91,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    rv = mListener->OnStopRequest(this, mContext, NS_ERROR_FILE_NOT_FOUND);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    nsCOMPtr<nsILoadGroup> loadGroup;
> +    rv = GetLoadGroup(getter_AddRefs(loadGroup));

also don't need GetLoadGroup here.

@@ +203,5 @@
> +  nsCOMPtr<nsIURL> url = do_QueryInterface(aUri);
> +  rv = url->GetFilePath(fileSpec);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  mozilla::dom::AppInfo appInfo;

where does mozilla::dom::AppInfo live?  Is it the .webidl file you create here?  Not sure how that magic works.  I assume you do :)

@@ +206,5 @@
> +
> +  mozilla::dom::AppInfo appInfo;
> +
> +  if (!mAppInfoCache.Get(host, &appInfo)) {
> +    nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID);

Looking through the tree it seems to be the case that we generally check the success of do_GetService.  But then some places don't.  Can't hurt to add a check?

::: netwerk/protocol/app/AppProtocolHandler.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

vim modeline too (and your emacs modeline is not canonical):

  https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Mode_Line

::: netwerk/protocol/app/AppProtocolHandler.js
@@ -51,5 @@
> -  },
> -
> -  newChannel: function app_phNewChannel(aURI) {
> -    // We map app://ABCDEF/path/to/file.ext to
> -    // jar:file:///path/to/profile/webapps/ABCDEF/application.zip!/path/to/file.ext

good comment--let's keep in in C++ code

@@ -68,5 @@
> -    }
> -
> -    let uri;
> -    if (this._runningInParent || appInfo.isCoreApp) {
> -      // In-parent and CoreApps can directly access files, so use jar:file://

also good comment worth keeping

Comment 7

[[:fabrice] Fabrice Desré]

Attachment: patch v3

Jason, I had to reimplement the DummyChannel from scratch to get rid of the crash. It's absolutely uninteresting, with most of of functions being just not implemented, but it works!

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 820594 [details] [diff] [review]
patch v3

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

::: b2g/chrome/content/shell.js
@@ +1461,5 @@
> +}, false);
> +xhr.addEventListener("error", function(event) {
> +  dump("XXX app://poppit/file.html error");
> +}, false);
> +xhr.send(null);

I think you let some of your debugging code here :)

Comment 9

[[:fabrice] Fabrice Desré]

Yes, but I don't plan to push it...

Comment 10

[Jason Duell]

Comment on attachment 820594 [details] [diff] [review]
patch v3

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

::: netwerk/protocol/app/AppProtocolHandler.cpp
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

missing emacs/vim modelines

@@ +45,5 @@
> +{
> + /* nsCOMPtr<nsIURI> appURI;
> +  NS_NewURI(getter_AddRefs(appURI),
> +            "app://unknown/nothing.html", nullptr, nullptr);
> +  SetOriginalURI(appURI);*/

Ditch the junk you've commented out.

@@ +131,5 @@
> +  }
> +
> +  mListener = nullptr;
> +  mListenerContext = nullptr;
> +  mPending = false;

So I see that Http/FTP channels both set mPending to false right before calling OnStopRequest.  Let's do that too.

@@ +133,5 @@
> +  mListener = nullptr;
> +  mListenerContext = nullptr;
> +  mPending = false;
> +  rv = SetNotificationCallbacks(nullptr);
> +  NS_ENSURE_SUCCESS(rv, rv);

nothing checks the return value from Runnable::Run.  No one can hear you scream.... :)

Also you've made it impossible (NOT_IMPLEMENTED) to set callbacks, so no need to clear them here?  Fine either way.

@@ +168,5 @@
> +  mLoadFlags = aLoadFlags;
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP DummyChannel::GetOriginalURI(nsIURI**)

I'm going to leave it to you to find out if things work OK with all these NOT_IMPLEMENTED methods.  If it works, fine!

::: netwerk/protocol/app/AppProtocolHandler.h
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

missing emacs/vim modelines

@@ +22,5 @@
> +  virtual ~AppProtocolHandler();
> +
> +  // Define a Create method to be used with a factory:
> +  static nsresult
> +    Create(nsISupports* aOuter, const nsIID& aIID, void* *aResult);

Uber-nit:  I'd generally indent the function arguments, not the function name.

Comment 11

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/0ab4e1cae487

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0ab4e1cae487

Comment 13

[Tom S [:evilpie]]

Comment on attachment 820594 [details] [diff] [review]
patch v3

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

::: netwerk/protocol/app/AppProtocolHandler.cpp
@@ +374,5 @@
> +      return NS_OK;
> +    }
> +
> +    mozilla::AutoSafeJSContext cx;
> +    if (!appInfo.Init(cx, JS::Handle<JS::Value>::fromMarkedLocation(&jsInfo)) ||

This looks bad, are you sure jsInfo is actually rooted? Maybe you wanted to use JS::RootedValue for jsInfo.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

What impact did this have on startup time?

Comment 15

[[:fabrice] Fabrice Desré]

(In reply to Tom Schuster [:evilpie] from comment #13)

> ::: netwerk/protocol/app/AppProtocolHandler.cpp
> @@ +374,5 @@
> > +      return NS_OK;
> > +    }
> > +
> > +    mozilla::AutoSafeJSContext cx;
> > +    if (!appInfo.Init(cx, JS::Handle<JS::Value>::fromMarkedLocation(&jsInfo)) ||
> 
> This looks bad, are you sure jsInfo is actually rooted? Maybe you wanted to
> use JS::RootedValue for jsInfo.

How can I check that it's rooted properly? It's coming from a call to an xpcom component - is that good enough?

Comment 16

[[:fabrice] Fabrice Desré]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #14)
> What impact did this have on startup time?

I hope we'll get numbers from datazilla, but overall since gaia moved to a model where they lazy load a lot of small files, the JS implementation slowness was in part due to never enter the JIT and in part xpconnect overhead. According to https://bugzilla.mozilla.org/show_bug.cgi?id=924337#c2 we can expect ~50ms for the dialer.

Comment 17

[Tom S [:evilpie]]

I am pretty sure XPCOM doesn't root stuff for you. Terrence probably knows.

Comment 18

[Terrence Cole [:terrence]]

Comment on attachment 820594 [details] [diff] [review]
patch v3

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

Great catch, Tom!

::: netwerk/protocol/app/AppProtocolHandler.cpp
@@ +374,5 @@
> +      return NS_OK;
> +    }
> +
> +    mozilla::AutoSafeJSContext cx;
> +    if (!appInfo.Init(cx, JS::Handle<JS::Value>::fromMarkedLocation(&jsInfo)) ||

Yes, if that is taking a JS::Handle then you absolutely need to root jsInfo yourself. I think you should be able to move |cx| up a bit and replace |JS::Value jsInfo;| with |JS::Rooted<JS::Value> jsInfo(cx);|. Please only use |fromMarkedLocation| on locations that you know for sure will be marked by some other mechanism.

Comment 19

[Ed Morley [:emorley]]

Not uplifting to aurora until bug 930836 is fixed & has koi+

Comment 20

[Gregor Wagner [:gwagner]]

Backed out to check if it fixes bug 931022, bug 930804 and bug 930545
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/29d1a0889288

Comment 21

[Jason Smith [:jsmith]]

(In reply to Gregor Wagner [:gwagner] from comment #20)
> Backed out to check if it fixes bug 931022, bug 930804 and bug 930545
> remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/29d1a0889288

The bisection points at this point being the regressing patch at least for bug 930545.

Anyways - this is a feature & based on the regressions we've already seen here, this way too risky to take into 1.2 at this point. This needs to ride the trains into 1.3.

Comment 22

[Ed Morley [:emorley]]

Double landed on mozilla-central to expedite the respins, at Jason's request:
https://hg.mozilla.org/mozilla-central/rev/2f2a45f04e7c

In the future the quickest way to get a backout in the Nightlies is to land directly on mozilla-central and then we can immediately respin (& also merge the other way).

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch from bug 930836

Since bug 930836 was closed, I figured I'd attach this patch here so we don't lose it. See also bug 930836 comment 6.

One thing I didn't do here but might be a good idea would be to use an nsClassHashtable instead of an nsDataHashtable for the map from app to appinfo. Doing so would avoid a bunch of copies of the appinfo struct.

Comment 24

[[:fabrice] Fabrice Desré]

The root issue of the regressions is that the app:// urls were marked as immutable while they should not.

I pushed to try with that fixed, and the change to use nsClassHashtable:
https://tbpl.mozilla.org/?tree=Try&rev=18fe49ed17aa

Comment 25

[[:fabrice] Fabrice Desré]

Attachment: patch v4

Blake, the only notable change here is the use of nsClassHashTable.

Comment 26

[[:fabrice] Fabrice Desré]

Attachment: mrbkap's patch rebased.

Feel free to redirect the review...

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Make sure the gJarHandler is initialized before we create a JAR channel.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 823469 [details] [diff] [review]
patch v4

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

::: netwerk/protocol/app/AppProtocolHandler.cpp
@@ +330,5 @@
> +
> +  nsCOMPtr<nsIURL> url(do_QueryInterface(surl, &rv));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  NS_ADDREF(*result = url);

This can be: url.forget(result);

Comment 29

[[:fabrice] Fabrice Desré]

Relanded:
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/f103eff52a1a
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/1c0e72d7cf8c

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f103eff52a1a
https://hg.mozilla.org/mozilla-central/rev/1c0e72d7cf8c

Comment 31

[Dietrich Ayala (:dietrich)]

Minusing, since if this was backed out, we wouldn't hold the release for it.

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

FYI, according to datazilla this appears to improve contacts app launch time by about 100ms.  You can see this in the time window when this landed before and then was backed out:

  http://mzl.la/1h131Lr

Comment 33

[Mike Lee [:mlee]]

100 ms improvement in Contacts is worth having. Noming this for 1.3 since launch latency improvements remain a FxOS Perf goal.

Comment 34

[Gregor Wagner [:gwagner]]

Well it's already in 1.3. Why are you nominating it?

Comment 35

[Mike Lee [:mlee]]

Great to hear. What information in this bug's info tells us that this is already included in 1.3?

Comment 36

[Gregor Wagner [:gwagner]]

(In reply to Mike Lee [:mlee] from comment #35)
> Great to hear. What information in this bug's info tells us that this is
> already included in 1.3?


It landed on master according to Comment29 so it is in 1.3