1021156 - Downloads can't always go to download manager; FL app requires special handling of two mime types

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Runtime, defect)

Description

[David Flanagan [:djf]]

Bug 1009781 removed the code that allowed apps to register activities to handle downloaded content.

Unfortunately, the implementatation of OMA Forward Lock depends on the FL app being invoked when the user clicks on a link with a Content-Type of vnd.oma.dd+xml or vnd.oma.drm.message.

So bug 1009781 caused a regression that completely breaks our Forward Lock implementation.

The simplest way to fix this is to revert bug 1009781 and then modify b2g/components/ContentHandler.js so that it only works for those two required mime types.

A better way to fix it would be to create a b2g/components/OMAHandler that was specific to those two mime types and was unrelated to activities.  I'm not sure who can write such a component, however. A bonus, though, is that if we did it this way, perhaps the handler could be made to launch the FL app directly rather than through an activity, which would obviate the need for the patch in bug 1020002.

In addition to getting a content handler back for theses OMA mime types, we also need changes to uriloader to ignore the content-disposition header for those mime types.  We need those links to go to the FL app regardless of the content-disposition sent by the server.

Also (and this is perhaps a topic for a separate bug) Antonio (who found this regression) noticed that bug 1009781 removed the code that maps activity handlers to content handlers, but did not remove the code in dom/activities/src/ActivitiesService that broadcasts the events resonsible for triggering the removed code.

Comment 1

[David Flanagan [:djf]]

Vivien: since Fabrice is away and since you were the reviewer for the bug that caused this regression, can you offer your opinion on how we should fix it?

Antonio, Aus, Gregor: who can work on fixing this regression?

Comment 2

[Antonio Manuel Amaya Calvo (:amac)]

I can do the changes... once we agree on what to change :)

Comment 3

[Jason Smith [:jsmith]]

Talking with Candice yesterday about this - Aus was looking into the root cause of this problem in bug 1017306's dependency.

Comment 4

[Aus Lacroix [:aus]]

:djf, I'll be working on that regression fix (which is really updating the PDF app to use blobs in it's launch activity and always send a blob if there is a file associated). I'm hoping to have a working patch today.

Comment 5

[David Flanagan [:djf]]

Note that the FL app expects a URL, not a blob, so a fix that invokes the FL app with a blob will still be a regression for Forward Lock.  And if the blob already exists then there are the FL DRM questions around where the blob is stored... If it gets saved to a file somewhere we don't have good DRM.

Comment 6

[David Flanagan [:djf]]

(In reply to Jason Smith [:jsmith] from comment #3)
> Talking with Candice yesterday about this - Aus was looking into the root
> cause of this problem in bug 1017306's dependency.

Thanks Jason. I read 1017306 and 1009780. Fixing those will still leave us with broken Forward Lock.  Forward locked content is transmitted unencrypted, and can not go to the download manager. Doing that will save it to a file which violates the forward lock spec.  We need to special case the two forward lock mime types so that urls get passed to the FL app and locked content is never saved as a file in the clear.

Comment 7

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 1. Ignore content-disposition attachment on B2G for OMA DRM files

This is the first part of the easiest solution: It just ignores the content-disposition header on B2G (#ifdef'ed) for OMA FL DRM files.

I didn't ask for review because I don't know who should review this (nor if this is the desired solution finally or not)

Comment 8

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2. Reenable activity mime handlers for OMA DRM files

And this is the second part. This reenables the ContentHandler, but restricts it only to the OMA drm types. Added them on an preference to ease testing (you can test with any type).

Note that most of this patch is just a backout of https://hg.mozilla.org/mozilla-central/rev/751e5080c233 (and that *just* backing that out would work as well). If you prefer me to part this on two patches (the backout and the changes over the backout let me know).

Comment 9

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

Comment on attachment 8435757 [details] [diff] [review]
Part 2. Reenable activity mime handlers for OMA DRM files

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

It seems overkill to re-enable this whole component for registering all mime type handler and just makes it works with one app.

Can't we instead have a custom component registered for the specified mime type (see how to do that at http://mxr.mozilla.org/mozilla-central/source/browser/components/BrowserComponents.manifest#6 ) and make it fire a systemMessage to the FL app ?

Comment 10

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2. Reenable activity mime handlers for OMA DRM files

Changed as per the previous review comment. Instead of using a system message (which would require a manifestURL or a broadcast) it uses the content-handler message to shell.js.

Comment 11

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8435755 [details] [diff] [review]
Part 1. Ignore content-disposition attachment on B2G for OMA DRM files

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

Gregor, I don't know if you can review this... if not, can you redirect to someone that can?

Comment 12

[Gregor Wagner [:gwagner]]

Comment on attachment 8435755 [details] [diff] [review]
Part 1. Ignore content-disposition attachment on B2G for OMA DRM files

bz is usually reviewing changes to this file.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8435755 [details] [diff] [review]
Part 1. Ignore content-disposition attachment on B2G for OMA DRM files

So... the comment doesn't really match the behavior.

The comment says we shouldn't hand off to an external handler, but the behavior is "whatever actually happens for that MIME type".

In particular, just canceling the load altogether would also avoid handing out the data to someone else, and is arguably more correct if the server is requiring that we not handle it ourselves and we also can't hand it over to someone else...

What, exactly, did we end up doing before bug 1009781?

If we _do_ want to handle this internally, seems like we want to do this on a lower level, because otherwise other parts of the system might get confused about whether it's an attachment or not...

Comment 14

[David Flanagan [:djf]]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #10)
> Created attachment 8436035 [details] [diff] [review]
> Part 2. Reenable activity mime handlers for OMA DRM files
> 
> Changed as per the previous review comment. Instead of using a system
> message (which would require a manifestURL or a broadcast) it uses the
> content-handler message to shell.js.

What does shell.js do with the content-handler message? Does this just launch the activity as before?  I love Vivien's idea of using a system message to invoke the FL app because that gets around the security issues of activities and allows us to revert the patch from bug 1020002 on master.  Do we have any precedent for hard-coding the manifest URLs of Gaia apps in b2g/components/?  If we do it elsewhere, then it seems like it would be okay to do it here.

Comment 15

[David Flanagan [:djf]]

(In reply to Boris Zbarsky [:bz] from comment #13)

> What, exactly, did we end up doing before bug 1009781?

A content handler got to take a crack at any content-disposition:inline downloads, and would invoke a view activity to open an appropriate viewer app if there was one.  content-disposition:attachment downloads were apparently just ignored or something.

This bug is in response to two things: the removal of the content handler, and also the discovery that some partners who rely on the forward lock implementation are using content-disposition:attachment
 
> If we _do_ want to handle this internally, seems like we want to do this on
> a lower level, because otherwise other parts of the system might get
> confused about whether it's an attachment or not...

Comment 16

[Boris Zbarsky [:bzbarsky]]

OK, so the content-disposition thing is not related to bug 1009781?

Typically, "content-disposition: attachment" on the web means "save this file".  We have partners who are using it to mean something else?  :(  What do they want done differently from the "content-disposition: inline" case?

Comment 17

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Boris Zbarsky [:bz] from comment #16)
> OK, so the content-disposition thing is not related to bug 1009781?

No, there are two different, but related, problems here, and each of the patches deal with part of the problem.

On one hand, bug 1009781 removed *all* the internal handlers for content types. That means that all content types that are not native get handled "externally" where "externally" means "by the download manager". And we don't want the OMA DRM files to be handled by the download manager. That's what the second patch on this bug is (to reenable the 'internal' processing for OMA DRM files).

Now, this patch deals with another part of the problem. Our OBs are indeed using content-disposition: attachment to send the files. Now, I don't know why they're doing that, but I know (now) that what should most definitely *not* happen is the default semantic for that (save the file as is).

I believe (I'm not sure, though) that they're sending C-D: attachment mostly because other platforms (Android?) might require that to ensure that the included browser does not handle the files directly.

What we did for 1.3 was to pass a version of this patch (basically the same without the ifdef) to the OEMS (we detected this *way* too late into the certification process) so it didn't matter if CD was inline, attachment or not present, in all cases the download would be handled by the FL application.

> Typically, "content-disposition: attachment" on the web means "save this
> file".  We have partners who are using it to mean something else?  :(  What
> do they want done differently from the "content-disposition: inline" case?

In fact, in both cases (both inline and attachment) for this kind of files the *same* process should happen. That process is to just delegate the processing of the file to the forward lock application. That's what this patch does (it makes forceExternalHandling=false for OMA DRM files, ensuring that they're not processed by the download manager).

Comment 18

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to David Flanagan [:djf] from comment #14)
> (In reply to Antonio Manuel Amaya Calvo (:amac) from comment #10)
> > Created attachment 8436035 [details] [diff] [review]
> > Part 2. Reenable activity mime handlers for OMA DRM files
> > 
> > Changed as per the previous review comment. Instead of using a system
> > message (which would require a manifestURL or a broadcast) it uses the
> > content-handler message to shell.js.
> 
> What does shell.js do with the content-handler message? Does this just
> launch the activity as before?  I love Vivien's idea of using a system
> message to invoke the FL app because that gets around the security issues of
> activities and allows us to revert the patch from bug 1020002 on master.  Do
> we have any precedent for hard-coding the manifest URLs of Gaia apps in
> b2g/components/?  If we do it elsewhere, then it seems like it would be okay
> to do it here.


shell.js do the same as it did before (and does now, that code wasn't changed): it invokes the activity. I don't think we have any Gaia manifestURL hardcoded in Gecko... and that Fabrice would cry if we add one while he's away :P. We could send a chrome-message to Gaia's system, and let system deal with invoking the FL app. But that would mean changes in system also... At this point I think it's way less risky to just take your patch to deal with preselecting FL for this activity, and use the invocation mechanism that we already have in place.

Comment 19

[Boris Zbarsky [:bzbarsky]]

OK.  So the forward lock application in this case is actually part of Gecko?

I'm basically trying to decide whether we should have an actual supported setup for doing this or whether to just go with the icky-but-simple ifdef approach.... ;)

Comment 20

[Antonio Manuel Amaya Calvo (:amac)]

The formard lock application is part of Gaia. What gecko does is to define a content handler for the Oma.drm type. And that that content handler does is just to pass the URL to shell.js, which in turn launches the fl application using an activity. But it only does that if we ignore the content disposition header, because otherwise the URL is passed to the download manager instead (which for all other cases is the right behavior but for Oma drm files it has the unfortunate effect of leaving the file stored unprotected on the device)

Comment 21

[Aus Lacroix [:aus]]

:amac, for the routing of the file to go to the download manager it must first go through the External Helper App Service and Handler in Gecko. That's the only way the download manager in gaia gets to see downloads.

If there's no way to realistically ignore content-disposition: attachment to route it to the oma drm content handler (and subsequent activity launch in gaia that starts the fl app) then we may be able to route the download from the download manager in gaia itself.

1. We can skip adding these files to the download history based on their content type.
2. When a file of that content type is downloaded successfully we can launch the fl app activity automatically.

What this doesn't do, is prevent apps that have permission to access the downloads api to see the download happening. However, the api is restricted to certified apps at this time.

Comment 22

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Ghislain Aus Lacroix [:aus] from comment #21)
> :amac, for the routing of the file to go to the download manager it must
> first go through the External Helper App Service and Handler in Gecko.
> That's the only way the download manager in gaia gets to see downloads.

And that's what it's doing now, even for inline downloads, because the activity handlers were removed. So everything that's not natively supported goes to the download manager.

> 
> If there's no way to realistically ignore content-disposition: attachment to
> route it to the oma drm content handler (and subsequent activity launch in
> gaia that starts the fl app) then we may be able to route the download from
> the download manager in gaia itself.
> 
> 1. We can skip adding these files to the download history based on their
> content type.
> 2. When a file of that content type is downloaded successfully we can launch
> the fl app activity automatically.
> 
> What this doesn't do, is prevent apps that have permission to access the
> downloads api to see the download happening. However, the api is restricted
> to certified apps at this time.

There's something else this does not prevent. The download manager stores the file on the SD card while it's being downloaded (and unless erased it stays there). And the user can get it from there (I know, it kinda sucks to have to protect something against the user/owner of the device but... it's DRM after all :)).

Comment 23

[Aus Lacroix [:aus]]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #22)
> (In reply to Ghislain Aus Lacroix [:aus] from comment #21)
> > :amac, for the routing of the file to go to the download manager it must
> > first go through the External Helper App Service and Handler in Gecko.
> > That's the only way the download manager in gaia gets to see downloads.
> 
> And that's what it's doing now, even for inline downloads, because the
> activity handlers were removed. So everything that's not natively supported
> goes to the download manager.

Ah yes, because those were removed we're missing our tie-in with the FL Apps registered handling of that content-type. Would it be terrible for us to add it back in but only specifically register the FL Apps oma/drm type? 

> > 
> > If there's no way to realistically ignore content-disposition: attachment to
> > route it to the oma drm content handler (and subsequent activity launch in
> > gaia that starts the fl app) then we may be able to route the download from
> > the download manager in gaia itself.
> > 
> > 1. We can skip adding these files to the download history based on their
> > content type.
> > 2. When a file of that content type is downloaded successfully we can launch
> > the fl app activity automatically.
> > 
> > What this doesn't do, is prevent apps that have permission to access the
> > downloads api to see the download happening. However, the api is restricted
> > to certified apps at this time.
> 
> There's something else this does not prevent. The download manager stores
> the file on the SD card while it's being downloaded (and unless erased it
> stays there). And the user can get it from there (I know, it kinda sucks to
> have to protect something against the user/owner of the device but... it's
> DRM after all :)).

Yep, that's also true, the .part file is created on the SD card in the downloads folder so a user could in theory copy the .part file before we move it to the secure storage location.

Comment 24

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Ghislain Aus Lacroix [:aus] from comment #23)
> (In reply to Antonio Manuel Amaya Calvo (:amac) from comment #22)
> > (In reply to Ghislain Aus Lacroix [:aus] from comment #21)
> > > :amac, for the routing of the file to go to the download manager it must
> > > first go through the External Helper App Service and Handler in Gecko.
> > > That's the only way the download manager in gaia gets to see downloads.
> > 
> > And that's what it's doing now, even for inline downloads, because the
> > activity handlers were removed. So everything that's not natively supported
> > goes to the download manager.
> 
> Ah yes, because those were removed we're missing our tie-in with the FL Apps
> registered handling of that content-type. Would it be terrible for us to add
> it back in but only specifically register the FL Apps oma/drm type? 

That's basically what the proposed patch (part 2) in this bug does ;)

> > > 
> > > If there's no way to realistically ignore content-disposition: attachment to
> > > route it to the oma drm content handler (and subsequent activity launch in
> > > gaia that starts the fl app) then we may be able to route the download from
> > > the download manager in gaia itself.
> > > 
> > > 1. We can skip adding these files to the download history based on their
> > > content type.
> > > 2. When a file of that content type is downloaded successfully we can launch
> > > the fl app activity automatically.
> > > 
> > > What this doesn't do, is prevent apps that have permission to access the
> > > downloads api to see the download happening. However, the api is restricted
> > > to certified apps at this time.
> > 
> > There's something else this does not prevent. The download manager stores
> > the file on the SD card while it's being downloaded (and unless erased it
> > stays there). And the user can get it from there (I know, it kinda sucks to
> > have to protect something against the user/owner of the device but... it's
> > DRM after all :)).
> 
> Yep, that's also true, the .part file is created on the SD card in the
> downloads folder so a user could in theory copy the .part file before we
> move it to the secure storage location.

And that's what the first patch on this bug is for... it makes the OMA types to go directly to the FL app always, no matter the content-disposition header.

Comment 25

[Aus Lacroix [:aus]]

:amac,

Gotcha. :) I think this solution is probably the most viable for 2.0.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8435755 [details] [diff] [review]
Part 1. Ignore content-disposition attachment on B2G for OMA DRM files

OK, I've thought about this, and I think this is the simplest thing to do after all.  I'd like the following change, though: name the allowContentDispositionToForceExternalHandling, always check it in the condition, and only have the ifdef around the setting of the boolean: on B2G set it to !mContentType.LowerCaseEqualsLiteral("application/vnd.oma.drm.message") and elsewhere set it to true.

r=me with those changes

Comment 27

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 1, V2. Ignore content-disposition attachment on B2G for OMA DRM files

Includes the comments from the previous review.

r=bzbarsky

Comment 28

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

Comment on attachment 8436035 [details] [diff] [review]
Part 2. Reenable activity mime handlers for OMA DRM files

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

R+ with nits.

::: b2g/components/OMAContentHandler.js
@@ +23,5 @@
> +const NS_ERROR_WONT_HANDLE_CONTENT = 0x805d0001;
> +
> +let OMAContentFactory = {
> +
> +}

Not sure you need this empty factory. Let's remove it.

@@ +42,5 @@
> +  },
> +
> +  handleContent: function handleContent(aMimetype, aContext, aRequest) {
> +    if (!(aRequest instanceof Ci.nsIChannel))
> +      throw NS_ERROR_WONT_HANDLE_CONTENT;

nit: Use {} around the throw instruction.

Comment 29

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

Clearing the needinfo as I already answered the questions.

Comment 30

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2, v2. Reenable activity mime handlers for OMA DRM files

r=vingtetun

Comment 31

[Antonio Manuel Amaya Calvo (:amac)]

Thanks for the reviews! Try run is at https://tbpl.mozilla.org/?tree=Try&rev=c100ff689149, waiting on it before requesting checkin

Comment 32

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 1, V2. Ignore content-disposition attachment on B2G for OMA DRM files

r=bzbarsky

I missed a ; that was hidden by LOG() being ""

Try run (only for the analysys) at: https://tbpl.mozilla.org/?tree=Try&rev=82114716efed

Comment 33

[Antonio Manuel Amaya Calvo (:amac)]

And the last run looks good (BTW, I don't know why it ran all the tests, I really only wanted to re-run the Hf). Maybe should have explicitly marked the none option (although since there's an all option also didn't thought it was necessary).

Anyway, requesting checkin.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #33)
> And the last run looks good (BTW, I don't know why it ran all the tests, I
> really only wanted to re-run the Hf). Maybe should have explicitly marked
> the none option (although since there's an all option also didn't thought it
> was necessary).
> 
> Anyway, requesting checkin.

It ran what you told it to run - "try: -b o -p all -u all -t none". But more importantly, you can just retrigger Hf on the existing push rather than doing a whole new push to Try. Please see the link below:
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Also, if you have any other questions about TBPL, please feel free to ping me on IRC. I'd be happy to help.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Oh, was looking at the wrong push. The reason is that what you specified - linux64-st-an - is for JS shell builds, which don't run on TBPL by default. "Hf" on TBPL is for the Firefox hazard analysis build - linux64-br-haz ("browser rooting analysis" on Trychooser).

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/bfc6270f98fe
https://hg.mozilla.org/integration/b2g-inbound/rev/7c41391a9bc7

Comment 37

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4][PTO June 19-22] from comment #34)
> (In reply to Antonio Manuel Amaya Calvo (:amac) from comment #33)
> > And the last run looks good (BTW, I don't know why it ran all the tests, I
> > really only wanted to re-run the Hf). Maybe should have explicitly marked
> > the none option (although since there's an all option also didn't thought it
> > was necessary).
> > 
> > Anyway, requesting checkin.
> 
> It ran what you told it to run - "try: -b o -p all -u all -t none". But more
> importantly, you can just retrigger Hf on the existing push rather than
> doing a whole new push to Try. Please see the link below:
> https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

But I had to make another push (I think) since I had to change a file (there was a missing ; the first time).

> 
> Also, if you have any other questions about TBPL, please feel free to ping
> me on IRC. I'd be happy to help.
Will do, thanks :)

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bfc6270f98fe
https://hg.mozilla.org/mozilla-central/rev/7c41391a9bc7

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9cc1f8dc93d3
https://hg.mozilla.org/releases/mozilla-aurora/rev/6619d09e8d80