383430 - Add API to XMLHttpRequest to provide convenience for background requests

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Manish Singh]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.4) Gecko/20070605 Firefox/2.0.0.4 Flock/0.8.99
Build Identifier: 

To provide some convenience for app or extension code doing requests in the background without user intervention, I propose the following:

1. Do not attach to a window's load group, so requests aren't cancelled if the window closes (often the window isn't really associated with the request anyway)
2. Do not get an nsIAuthPrompt by default, since we don't want auth dialogs to pop up randomly.
3. Provide an nsIBadCertListener implementation, again to prevent bad cert dialogs to pop up randomly.
4. Provide a mechanism to set an arbitrary string to be sent as an HTTP header or perhaps in the UA, so servers have an idea what components or extensions are causing requests.

We've done #1 and #2 already in the Flock tree.

Reproducible: Always

Comment 1

[Manish Singh]

To clarify, #1-3 would be turned on via a single boolean flag in the interface. In Fow now, in Flock I've called the flag backgroundRequest.

Comment 2

[Manish Singh]

In light of bug #327181, I don't think #3 is necessary anymore.

Comment 3

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

Are we talking about enabling this for content? Or just for chrome?

Comment 4

[Ian McKellar]

chrome only

Comment 5

[Manish Singh]

Yeah, chrome only. #2 and #4 *may* be useful in light of cross-site XHR support, but let's just get something hashed out for chrome users first.

Comment 6

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

You should be able to do 4 already using the normal SetRequestHeader API. Code with chrome access should be able to set the UA header.

Comment 7

[Manish Singh]

Spun off bug #405904 to cover #4.

Comment 8

[Manish Singh]

And #3 is relevant again with current trunk UI, as dialogs do pop up for bad certs.

Comment 9

[Manish Singh]

Attachment: Implement backgroundRequest behavior

This implements #1-3, and makes the extension manager and update service use it. 

For the bad cert stuff, I turned toolkit/mozapps/shared/src/badCertHandler.js into a JS component, and XMLHttpRequest references that accordingly, because content can't (won't?) depend on security interfaces. If the component isn't there, it falls back to the current behavior.

Extension manager and update server still use badCertHandler.js for the checkCert function, perhaps nsBadCertHandler.js could implement an nsIBadCertHandler interface, which provides checkCert? Then the JS #include could go away entirely. Given that there are already 3 users in the tree, it wouldn't be a big stretch to think other users could benefit.

Comment 10

[Manish Singh]

Oh, and currently badCertHandler.js doesn't implement nsIBadCertListener2, so it's not doing its full job anymore.

Comment 11

[Manish Singh]

Attachment: Implement backgroundRequest behavior, take 2

I forgot to limit it to chrome context only, plus I limited setting the flag to before open() is called, not because it's currently required, but just to give the freedom to apply behavior that might require that in the future.

Comment 12

[Manish Singh]

Attachment: Implement backgroundRequest behavior, take 3

Doh, I changed the rules of when backgroundRequest could be set without updating the callers, here's a fixed version.

Comment 13

[Manish Singh]

Attachment: Patch respun against current code

Comment 14

[Manish Singh]

Attachment: Another patch respin

Comment 15

[Manish Singh]

Attachment: New patch that incorporates changes discussed on IRC

Comment 16

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

Hmm.. with so little code left, does it really make sense to write the badcerthandler in javascript? Seems simpler to just write C++ and put it in nsXMLHttpRequest.cpp

Sorry, didn't think about that until now :(

Also, I'm thinking we should really have some security folks look at this. I don't really know certificate handling and since it's pretty important stuff I wanna make sure we do the right thing.

Comment 17

[Manish Singh]

Yeah, it still needs to be in JS. I originally wanted to do it all in C++ (even before, the code was not that complex), but the security IDLs get built long after layout, and the source even *depends* on layout and dom, so keeping it in JS avoids making a circular compile time dependency.

nsIBadCertListener2 and nsISSLErrorListener are single function interfaces, and the docs clearly state that returning true suppresses error dialogs but still cancel the request.

http://mxr.mozilla.org/firefox/source/security/manager/ssl/public/nsIBadCertListener2.idl#59
http://mxr.mozilla.org/firefox/source/security/manager/ssl/public/nsISSLErrorListener.idl#56

Flagging dveditz for review, since the cert stuff is based off his work.

Comment 18

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I really like the idea of this patch; should make things a fair bit easier for a lot of extension authors.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 304923 [details] [diff] [review]
New patch that incorporates changes discussed on IRC

r=dveditz on the badcert part.

Comment 20

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

Comment on attachment 304923 [details] [diff] [review]
New patch that incorporates changes discussed on IRC

Dan, do we really want to just ignore any bad certificates? Shouldn't we refuse to load the resource in that case?

Resetting review request to make sure it gets reviewed for that issue too.

Comment 21

[Manish Singh]

We *are* refusing to load resources with bad certs. The difference between behavior of the code here and what's in UpdateService/EM is that the latter disallows user whitelisted certs. I filed bug #418992 for that functionality.

Comment 22

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

Comment on attachment 304923 [details] [diff] [review]
New patch that incorporates changes discussed on IRC

r=me on the non-cert stuff, sr=me for the whole thing.

Comment 23

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 304923 [details] [diff] [review]
New patch that incorporates changes discussed on IRC

a=shaver, but two things:

1) please get some tests for this into the tree ASAP; if it's important enough to get into b5, it's important enough to have test coverage

2) what effect will this have on content that uses .backgroundRequest as an expando, perhaps to track their own "background" sorts of requests?  (My guess is that they get a cryptic security error now.)  Perhaps .mozBackgroundRequest would be a better name, since the property isn't _useful_ unless you're privileged, and we have no intent to standardize.

Comment 24

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 304923 [details] [diff] [review]
New patch that incorporates changes discussed on IRC

No, I've changed my mind here -- I think we need to at least rename the property name to .mozBackgroundRequest to avoid changing content-visible behaviour between last beta and RC.

Should be an easy fix, and I'll quickly approve the result.

Comment 25

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

I don't think it's that big a risk that someone will set expandos, but there's no harm in using a .moz prefix either, especially given that this is very unlikely to ever be used by web content.

And definitely write some tests, sorry should have thought of that myself.

Comment 26

[timeless]

+ * The Original Code is the Update Service.
+ *
+ * The Initial Developer of the Original Code is Ben Goodger.
+ * Portions created by the Initial Developer are Copyright (C) 2004

did you really take enough code to merit this?

Comment 27

[Manish Singh]

(In reply to comment #26)
> + * The Original Code is the Update Service.
> + *
> + * The Initial Developer of the Original Code is Ben Goodger.
> + * Portions created by the Initial Developer are Copyright (C) 2004
> 
> did you really take enough code to merit this?

All that code is from toolkit/mozapps/shared/src/badCertHandler.js. The only bits I added were to turn it into a real component via XPCOMUtils.

Comment 28

[Manish Singh]

(In reply to comment #25)
> And definitely write some tests, sorry should have thought of that mysel

So I'm not really sure how to test this. How would I go about writing a test to verify an auth prompt dialog or a security warning dialog *isn't* showing up?

The load group stuff is easy enough to test though.

Comment 29

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Add a listener that counts how many times it's called, and check that the count is zero when the request completes?

Comment 30

[Manish Singh]

Ok, bug #422808 made the auth prompt a non-issue anyway.

I'm not sure what the best way to trigger an invalid cert warning from the test framework.

Comment 31

[Manish Singh]

Hm, I'm unable to get a cert warning dialog from within the mochitest framework at all. Same code called from within a normal browser instance throws up a dialog.

Comment 32

[Manish Singh]

Attachment: New patch, with mozBackgroundRequest attribute

Ok, I've renamed the attribute to mozBackgroundRequest, and synced up the code with current CVS. As noted before, the auth prompt logic isn't needed anymore.

I've written a test for loadGroup detachment, but I can't seem to even get a security dialog to trigger from mochitest land. Can this be landed for b5, with a test for the security dialog stuff happening later once we figure out what's going on there?

Comment 33

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 310343 [details] [diff] [review]
New patch, with mozBackgroundRequest attribute

a=shaver, please file a bug on the needed test infrastructure, and mark it blocking a bug that describes the specific tests you feel are missing.

Comment 34

[Matthew (lilmatt) Willis]

Checking in content/base/public/nsIXMLHttpRequest.idl;
/cvsroot/mozilla/content/base/public/nsIXMLHttpRequest.idl,v  <--  nsIXMLHttpRequest.idl
new revision: 1.39; previous revision: 1.38
done
Checking in content/base/src/Makefile.in;
/cvsroot/mozilla/content/base/src/Makefile.in,v  <--  Makefile.in
new revision: 1.121; previous revision: 1.120
done
RCS file: /cvsroot/mozilla/content/base/src/nsBadCertHandler.js,v
done
Checking in content/base/src/nsBadCertHandler.js;
/cvsroot/mozilla/content/base/src/nsBadCertHandler.js,v  <--  nsBadCertHandler.js
initial revision: 1.1
done
Checking in content/base/src/nsXMLHttpRequest.cpp;
/cvsroot/mozilla/content/base/src/nsXMLHttpRequest.cpp,v  <--  nsXMLHttpRequest.cpp
new revision: 1.229; previous revision: 1.228
done
Checking in content/base/test/Makefile.in;
/cvsroot/mozilla/content/base/test/Makefile.in,v  <--  Makefile.in
new revision: 1.63; previous revision: 1.62
done
RCS file: /cvsroot/mozilla/content/base/test/test_bug383430.html,v
done
Checking in content/base/test/test_bug383430.html;
/cvsroot/mozilla/content/base/test/test_bug383430.html,v  <--  test_bug383430.html
initial revision: 1.1
done

Landed on trunk

Comment 35

[Manish Singh]

Will file bugs about test framework.

Marking bug as FIXED as code has landed.

Comment 37

[Mark Finkle (:mfinkle) (use needinfo?)]

updated http://developer.mozilla.org/en/docs/XMLHttpRequest#mozBackgroundRequest

Comment 38

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

Was it intended that doing a background request should always suppress auth prompts? If so that isn't working I realized. Making GetInterface not return an nsIAuthPrompt here simply means that we'll try to get one from the loadgroup, which in many cases will mean that the docshell will provide one.

Although currently chrome docshells most of the time don't provide authprompts, except if a proxy requests authentication.

Comment 39

[Manish Singh]

(In reply to comment #38)
> Although currently chrome docshells most of the time don't provide authprompts,
> except if a proxy requests authentication.

Hm, since when you enable mozBackgroundRequest you don't get the document's loadgroup, would you still get a proxy auth dialog?

Comment 40

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

Ah, good point, no you don't

Comment 41

[Frank Wein [:mcsmurf]]

Does a normal Gecko app like FF or SeaMonkey need to ship nsBadCertHandler.js? Just wondering because this file isn't included in browser/installer/windows/packages-static or other packaging files.

Comment 42

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

Yes. If that's not shipped inside a jar or elsewhere that seems like a bug.

Comment 43

[Manish Singh]

Should be simple enough to add to packages-static.. shall I open a new bug or reopen this one?

Comment 44

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

New bug, please.

Comment 45

[Manish Singh]

Filed bug #452135.