957592 - Downloads API needs to pass DOM peer review

Status: RESOLVED FIXED

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

Description

[Peter Van der Beken [:peterv]]

See bug 938023 comment 70.

Not sure why this wasn't filed when the backout of bug 938023 was undone, but we need this done sooner rather than later.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Isn't the download API in 1.3?  If so, this needs to be 1.3+.

Comment 2

[Gregor Wagner [:gwagner]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #1)
> Isn't the download API in 1.3?  If so, this needs to be 1.3+.

The download manager/API for b2g is a 1.4 feature and we backed everything out in 1.3 afaik.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ah.  Bug 938023 has a target milestone set indicating it's in 1.3, but it was really backed out in bug 948040.

Comment 4

[Gregor Wagner [:gwagner]]

Peter, how do you want to do this?

Comment 5

[Aus Lacroix [:aus]]

So, guys, where are we with this? This should be completed this week.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Where we are is that someone needs to write a patch to fix the issues pointed out in the comment referenced in comemnt 0 of this bug.  Once that someone (you?  Fabrice?  Someone else involved in bug 938023?) does that, along with the corresponding changes to test_interfaces.html, Peter or I review the patch and it gets checked in and then backported to whatever branches bug 938023 is on.

Comment 7

[Boris Zbarsky [:bzbarsky]]

And I'm happy to hand-hold as needed, but to do that I need to understand what part of bug 938023 comment 70 is not clear.

Comment 8

[Gregor Wagner [:gwagner]]

Adding comment 70 from bug 938023

(In reply to Peter Van der Beken [:peterv] from comment #70)
> Comment on attachment 8339529 [details] [diff] [review]
> Patch - Final - Download API (Test Interfaces Settings) - Apply Last
> 
> Review of attachment 8339529 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So this is just the test change, but really it's about exposing those
> interfaces. It seems odd that we're exposing these interfaces everywhere (on
> B2G), they're only usable if the "downloads" permission is set? If that's
> the case I think the interfaces should have a |Func="..."| extended
> attribute calling a C++ function that checks for the "downloads" permission
> (similar to what the code in Navigator::DoNewResolve does).
> 
> I'll file a bug on making this nicer, we should be able to actually generate
> these permission checks from an extended attribute on the interface. But
> that shouldn't block this for now.

The thing is that we have to expose the navigator property even if an app doesn't have the permission. The marketplace app has to be able to determine if an API is available or not even if it doesn't have the permission to access it.
AFAIK the current deal with the marketplace app is that the navigator property is 'undefined' if an API is not available on the device (controlled by a pref). It is 'null' if it is available but the marketplace app doesn't have the permission to access it.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I thought we'd come up with a setup for the marketplace app that doesn't force use to pollute the web with this stuff.  Certainly plenty of other permissions-based stuff on navigator is hidden based on those permissions!

Comment 10

[Boris Zbarsky [:bzbarsky]]

But also, I don't see what Peter's comments have to do with the marketplace app.  We're not saying to make navigator.mozDownloadManager invisible to the web, though that would be ideal.  Presumably that's what marketplace uses to detect the API, right?

What Peter's comment is talking about is window.DOMDownload, window.DOMDownloadManager, window.DownloadEvent.  Those are being stuck on the window for all webpages.  That's not OK, and certainly isn't needed by marketplace.

Comment 11

[Boris Zbarsky [:bzbarsky]]

And if marketplace _does_ currently still need the namespace pollution on Navigator, who owns fixing that problem?  We've been talking about it for like 18 months now, and our current setup really is not acceptable if it still requires exposing random broken stuff to the web just so marketplace can detect it.  It's just not.

Comment 12

[Gregor Wagner [:gwagner]]

(In reply to Boris Zbarsky [:bz] from comment #10)
> 
> What Peter's comment is talking about is window.DOMDownload,
> window.DOMDownloadManager, window.DownloadEvent.  Those are being stuck on
> the window for all webpages.  That's not OK, and certainly isn't needed by
> marketplace.

Ok I see what you mean now. Thanks for the clarification.
A quick followup question: What is different with the download API and all the other APIs we are exposing for B2G that are behind a permission? I assume they got a proper r+ from a DOM peer.
Are we at a point where we need a generic solution or is there anything else I am missing here.

For the API detection for marketplace: I thought Jonas has a plan there. I will let him talk to this once he is back.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> What is different with the download API and all the other APIs we are exposing for B2G
> that are behind a permission?

There shouldn't be any.

> I assume they got a proper r+ from a DOM peer.

I wouldn't assume that.  People have been checking in stuff without review all over; that's why the scary warnings got added to test_interfaces that bug 938023 then ignored.  We may well need to go and fix other APIs too.

> Are we at a point where we need a generic solution

Maybe, see bug 938023 comment 70.  But I wouldn't block on that generic solution for fixing this bug...

Comment 14

[Aus Lacroix [:aus]]

OK, sounds like I have enough information to wrap this up (in lieu of a generic solution). Fabrice had a preliminary patch to address some of the concerns. I will see if he still has it laying around and will revive it.

Comment 15

[Aus Lacroix [:aus]]

Attachment: Patch - Address review concerns from DOM Peers.

Comment 16

[Aus Lacroix [:aus]]

Attachment: Patch - Final - Address review concerns from DOM Peers.

Comment 17

[Andrew McCreight [:mccr8]]

khuey, peterv or bz may make more sense as reviewers, as they are probably more familiar with this.

Comment 18

[Boris Zbarsky [:bzbarsky]]

>+    // WebIDL standard specifies we should simply ignore setting attributes
>+    // to invalid enum values.

The bindings actually enforce that already.

But in any case, "state" is readonly, so who's going to be calling that setter?

And even more importantly, that patch seems to be completely ignoring the main issue this bug was filed about...

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8360690 [details] [diff] [review]
Patch - Final - Address review concerns from DOM Peers.

r- due to comment 18.

To be clear, it appears the missing piece here is that the interface names are always visible, even to code that doesn't have the "downloads" permission.

Comment 20

[Reuben Morais [:reuben]]

What would be a good place to stick a [Func] in this case, since it's a JS-implemented interface?

Comment 21

[Aus Lacroix [:aus]]

In Navigator.cpp in dom/base. I'm testing out a patch that addresses the visibility of mozDownloadManager to apps that don't have the correct permissions right now, it'll be up as soon as my mochitest run passes.

Comment 22

[Aus Lacroix [:aus]]

Attachment: Patch - v2 - bug957592-domdownload-webidl.patch

Comment 23

[Aus Lacroix [:aus]]

Attachment: bug957592-domdownload-webidl.patch

Comment 24

[Boris Zbarsky [:bzbarsky]]

Please don't forget to revert the test_interfaces.html changes from bug 938023....

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8361404 [details] [diff] [review]
bug957592-domdownload-webidl.patch

>+  nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(js::CheckedUnwrap(aGlobal));

Why do you need the CheckedUnwrap?  That seems fishy...

r=me on the rest if you revert attachment 8339529 [details] [diff] [review] and if the marketplace is ok with this setup.  Thank you!

Comment 26

[Reuben Morais [:reuben]]

(In reply to Ghislain 'Aus' Lacroix from comment #21)
> In Navigator.cpp in dom/base.

I thought the plan was to leave navigator.mozDownloadManager as null, just not the stuff on window. That would mean the Navigator::HasDownloadsSupport method would be unrelated to Navigator. I guess if the null-means-no-permission thing is no longer necessary, I'll file a bug to remove the mozSettings code.

Comment 27

[Aus Lacroix [:aus]]

Boris,

I believe I need to do this because part of the implementation accesses the content window from chrome. Which ends up being a security wrapper around a window. This means that when we get to getting the window from the global, it's possible that we will need to unwrap first. I believe if it's already the window unwrap does nothing. I just learned how all this really works today, so, if I got some parts in there a little off, I apologize :)

I will revert attachment 8339529 [details] [diff] [review] as well as part of the final patch.


Reuben,

From my understanding, yes, we could update Settings the same way.

Comment 28

[Boris Zbarsky [:bzbarsky]]

> I believe I need to do this because part of the implementation accesses the content
> window from chrome.

Hmm.  So you have chrome code doing someContentWindow.DOMDownload, basically?  But do you really want that to return undefined if someContentWindow doesn't have the permission?  Seems like it would make more sense for chrome to always see the DOMDownload property on someContentWindow, no matter what the window's permissions are, I'd think.

Comment 29

[Aus Lacroix [:aus]]

Yes, that's correct. the chrome code will be using the constructor attached to the window. I agree that it would be ideal for the chrome window / js code to always have access to it. I'm not sure what the best way to do this would be, could you give me some pointers?

Comment 30

[Boris Zbarsky [:bzbarsky]]

mozilla::dom::ThreadsafeCheckIsChrome should do the trick.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Though given this is window stuff, just xpc::AccessCheck::isChrome(aObj) would work too.

Comment 32

[Aus Lacroix [:aus]]

Attachment: Patch - v3 - Minor updates based on review

r+ carries. bz, if you don't mind having a quick look, it'd be much appreciated! :)

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8363311 [details] [diff] [review]
Patch - v3 - Minor updates based on review

>+++ b/dom/base/Navigator.cpp
>+  if(mozilla::dom::ThreadsafeCheckIsChrome(cx, aGlobal)) {

Space after "if".  And drop the "mozilla::dom::" bit: this file is already in that namespace.

Also, please name the first argument to this function "aCx", since the second one is aGlobal.

>+++ b/dom/base/Navigator.h
+  static bool HasDownloadsSupport(JSContext* /* unused */, JSObject* aGlobal);

The first argument is used.

>+++ b/dom/downloads/tests/serve_file.sjs

Bunch of missing whitespace in here ("catch(e)", "if(" a few times, etc).

The rest looks fine, I think.

Comment 34

[Aus Lacroix [:aus]]

Attachment: Patch - Final - Address review concerns from DOM peers.

r+ carries from bz. Thanks for all the help. :)

Comment 35

[Aus Lacroix [:aus]]

Attachment: Patch - Final - Address review concerns from DOM peers.

r+ carries from bz.

Comment 36

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/b2g-inbound/rev/786d44e03d8b

Comment 37

[Aus Lacroix [:aus]]

Inbound is green, marking fixed.

Comment 38

[Gregor Wagner [:gwagner]]

(In reply to Ghislain 'Aus' Lacroix from comment #37)
> Inbound is green, marking fixed.

Not so fast. The sheriffs are doing this once they merge to central :)

Comment 39

[Aus Lacroix [:aus]]

Whoops! Gotcha! I'm just excited that it's finally landed and it looks like I didn't break anything! :)

Comment 40

[Gregor Wagner [:gwagner]]

Comment on attachment 8363871 [details] [diff] [review]
Patch - Final - Address review concerns from DOM peers.

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

::: dom/downloads/tests/test_downloads_navigator_object.html
@@ +47,5 @@
>    function() {
>      SpecialPowers.pushPermissions([
>        {type: "downloads", allow: 0, context: document}
>      ], function() {
> +      ise(frames[0].navigator.mozDownloadManager, undefined, "navigator.mozDownloadManager is undefined when the page doesn't have permissions");

Oh we shouldn't do that. The navigator.mozDownloadManager has to stay the same when we don't have permission :(

Comment 41

[Aus Lacroix [:aus]]

OK. It sounds like what I'd have to do is keep the previous code that existed in Navigator.cpp that exposed mozDownloadManager as null even when the app does not have permissions but also keep the new function style check for DOMDownload and DOMDownloadEvent?

Comment 42

[Aus Lacroix [:aus]]

This is the code in question that I believe I need to re-insert for Marketplace feature detection:

-      if (name.EqualsLiteral("mozDownloadManager")) {
-        if (!CheckPermission("downloads")) {
-          aValue.setNull();
-          return true;
-        }
-      }

Correct?

Comment 43

[Carsten Book [:Tomcat]]

Hey Guys,

sorry had to backout this change because of hazard analysis permafailure/bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=33451782&tree=B2g-Inbound

Comment 44

[Boris Zbarsky [:bzbarsky]]

The analysis thinks (incorrectly, I suspect) that ThreadSafeCheckIsChrome can gc.  Since aGlobal is used after that call, it needs to be locally rooted...

Comment 45

[Aus Lacroix [:aus]]

Attachment: Patch - Keep compatibility with Marketplace while hiding the rest.

OK. It looks like we definitely still need to expose mozDownloadManager on navigator when the pref is turned on but the permission isn't present. I've gone ahead and done that to the best of my knowledge.

I've also attempted to solve the hazard analysis failure by rooting the object before calling CheckIsChrome and for safety am using it during other calls as well.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8364659 [details] [diff] [review]
Patch - Keep compatibility with Marketplace while hiding the rest.

>+  // We'll need a rooted object so that no GC is possible while we're calling
>+  // CheckIsChrome.

No, you need it so it won't go away if ThreadsafeCheckIsChrome GCs.  You can't _prevent_ GC with a Rooted.

r=me with the comment fixed, assuming this passes test_interfaces.html.

Comment 47

[Aus Lacroix [:aus]]

Attachment: Patch - Final - Keep compatibility with Marketplace while hiding the rest

r+ carries over from bz. Still waiting on review from Gregor.

Comment 48

[Aus Lacroix [:aus]]

FYI, I ran test-interfaces locally and it did not fail because of any of the downloads api interfaces. It did fail because it was lacking interfaces that are not available on desktop. I think we're in the green there.

Comment 49

[Gregor Wagner [:gwagner]]

Comment on attachment 8364732 [details] [diff] [review]
Patch - Final - Keep compatibility with Marketplace while hiding the rest

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

::: dom/webidl/Downloads.webidl
@@ +19,5 @@
> +
> +//
> +// XXXTODO: When we have a generic way to do feature detection in marketplace
> +//          we will *STOP* using the pref and use the function like DOMDownload
> +//          and DownloadEvent. 

nit: Whitespace

Comment 50

[Gregor Wagner [:gwagner]]

Hm the interface tests are failing: https://tbpl.mozilla.org/php/getParsedLog.php?id=33507864&tree=Pine&full=1

Comment 51

[Reuben Morais [:reuben]]

Do you perhaps want DOMDownloadManager to be [NoInterfaceObject]?

Comment 52

[Boris Zbarsky [:bzbarsky]]

Yes, I think you do.

Comment 53

[Aus Lacroix [:aus]]

Attachment: Patch - Keep compatibility with Marketplace, Hide Interface Object on DOMDownloadManager

r+ carries from bz and gwagner.

Comment 54

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/b2g-inbound/rev/2ca83cb93f50

Comment 55

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2ca83cb93f50

Comment 56

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

For certified APIs, there is no reason for marketplace (or other similar consumers) to be able to detect support for the API. Marketplace will never host apps that can use certified APIs anyway.

For non-certified APIs, I was talked out of exposing a null-valued property.

It was deemed "too non-javascripty" to ask people to do "if (navigator.foo)" to detect ability to use and "if ('foo' in navigator)" to detect implemented-but-you-might-not-have-access.

Instead we were going to add some sort of hasFeature('api.navigator.foo')-like API which we'd use to allow specifically for implemented-but-you-might-not-have-access. Lots of unanswered questions here though. But this seems to me like the path forward. Details to be discussed in a separate bug obviously.

Comment 57

[Boris Zbarsky [:bzbarsky]]

> I believe I need to do this because part of the implementation accesses the content
> window from chrome.

Hmm.  I was just thinking about this some more.  Blake says we should really only expose contentWindow.DOMDownload if the target window has the permission.  So maybe the CheckedUnwrap thing is in fact the right thing to do...

Comment 58

[Boris Zbarsky [:bzbarsky]]

Blake, see comment 27 to comment 29.

Comment 59

[Aus Lacroix [:aus]]

As soon as you guys let me know which is ideal I'll create a small follow-up bug and attach a patch (if need be :))

Comment 60

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #57)
> Hmm.  I was just thinking about this some more.  Blake says we should really
> only expose contentWindow.DOMDownload if the target window has the
> permission.  So maybe the CheckedUnwrap thing is in fact the right thing to
> do...

My understanding is that the immediate need for chrome to access content's DownloadManager's interface only happens in cases where either solution works. Beyond that, I think that Xrays should reflect the actual state of the underlying DOM object, so that CheckedUnwrap makes more sense here.

Comment 61

[Boris Zbarsky [:bzbarsky]]

OK.  I'll do that as part of fixing bug 965094.