Closed Bug 800799 Opened 12 years ago Closed 12 years ago

http-on-modify-request now called after JS that started the load is no longer on the stack

Categories

(Core :: Networking: HTTP, defect)

x86
Windows Vista
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla20
Tracking Status
firefox17 --- unaffected
firefox18 --- fixed
firefox19 --- verified
firefox20 --- verified

People

(Reporter: Honza, Assigned: jduell.mcbugs)

References

Details

(Whiteboard: [need review][firebug-p1])

Attachments

(1 file, 6 obsolete files)

One of the Firebug automated tests started failing in Nightly.

STR:

1) Install Firebug:
https://getfirebug.com/releases/firebug/1.11/firebug-1.11.0a4.xpi

2) Load:
https://getfirebug.com/tests/head/net/breakpoints/breakOnXHR.html

3) Follow instructions on the page. The breakpoint doesn't hit BUG

It works in Firefox 17
Fails in Firefox 19

---

You can also install FBTrace and see the ERROR
Download from:
https://getfirebug.com/releases/fbtrace/1.10/fbTrace-1.10b3.xpi

Open the console before the test using: Firebug (icon) menu -> Open Firebug Tracing

The error comes from here:
resource://firebug/firebug-service.js (1922)

https://github.com/firebug/firebug/blob/master/extension/modules/firebug-service.js#L1915

After the frames coming from chrome scope are pealed off, there is none left in Fx19

Honza
How about FF18?  IonMonkey came with some changes to the debugger's view of the stack (viz., JS_BrokenFrameIterator).
(In reply to Jan Honza Odvarko from comment #0)
> One of the Firebug automated tests started failing in Nightly.
> 
> It works in Firefox 17
> Fails in Firefox 19

Can you precise which version of Firefox 19 you have used, I landed a patch recently to prevent iterating over Ion frames, because the current API does not abstract enough the JS stack representation to allow us to do so safely.

The current work around with the recent Debugger API is to disable IonMonkey compilation in the compartment, I would have expected the same with JSD.
Testing with Nightly (Fx19):
Built from http://hg.mozilla.org/mozilla-central/rev/57304bbf9c0e
- the problem is there

Testing with Aurora (Fx18):
Built from http://hg.mozilla.org/releases/mozilla-aurora/rev/0f13f4e81d94
- the problem is there

Honza
Ok, I can reproduce it with Firebug 1.10.4 and this is not an IonMonkey Bug (still persist after disabling ion.content).  The test case is the following function, with a breakpoint set on sent request to https://getfirebug.com/tests/head/net/breakpoints/process1.php?param=1 from the Net panel.

function executeRequest(value)
{
  var request = new XMLHttpRequest();
  request.open("POST", "process1.php?param=" + value, true);
  request.send(null);
}

I don't know how you are setting a breakpoint on a net request, can you detail how the breakpoint is set?

XMLHttpRequest is using a new DOM bindings and this has been modified to add some optimization to IonMonkey. I don't know if this might be related even if I think it is unlikely. (CC: bz)
Should be pretty unlikely, yes.  The new DOM bindings just change which native is called when open() and send() happen, which should not affect JS stacks, generally....

Jan, can you find a one-day regression range for this?
> I don't know how you are setting a breakpoint on a net request, can you detail 
> how the breakpoint is set?
Honza's test case already mentions how to set an XHR breakpoint. Just switch to the Net panel and enable it, click the "Execute Test" button on the page and then the breakpoint column besides the XHR.

Sebastian
(In reply to Sebastian Zartner from comment #6)
> > I don't know how you are setting a breakpoint on a net request, can you detail 
> > how the breakpoint is set?
> Honza's test case already mentions how to set an XHR breakpoint. Just switch
> to the Net panel and enable it, click the "Execute Test" button on the page
> and then the breakpoint column besides the XHR.

The question is, which API function is used when you are clicking in the breakpoint column?
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #7)
> (In reply to Sebastian Zartner from comment #6)
> > > I don't know how you are setting a breakpoint on a net request, can you detail 
> > > how the breakpoint is set?
> > Honza's test case already mentions how to set an XHR breakpoint. Just switch
> > to the Net panel and enable it, click the "Execute Test" button on the page
> > and then the breakpoint column besides the XHR.
> 
> The question is, which API function is used when you are clicking in the
> breakpoint column?
When you click on the breakpoint column, nothing special happens (at least from JSD perspective).

A flag is created saying: break into the debugger when XHR with the same URL happens again.

If such XHR happens, Firebug checks whether there is the flag. This happens within "http-on-modify-request" handler.

If the check returns true (the flag is there), Firebug calls a function that has "debugger;" keyword in it, see:
https://github.com/firebug/firebug/blob/master/extension/modules/debuggerHalter.js

This causes the debugger (JSD) to break.

Consequently, the logic is looking for the first frame (youngest) coming from the content JS (page) and pointing the Scrip panel to it.

The problem (this bug report) is that there is no such frame (coming from the content) and so, the break doesn't happen.


Honza
OK, so the regression range from comment 8 is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=aa5e3b445810&tochange=ec10630b1a54

There are some debugger changes in there, looks like, but nothing else obviously relevant...
It's just a wild guess, but:

Bug 797684 - nsIProxiedChannel.proxyInfo being unavailable to http-on-modify-request observers causes NoScript (and possibly other extensions) to break navigation or cause DNS leaks in some proxied configurations.

Instead of calling gHttpHandler->OnModifyRequest(this); in AsyncOpen it's called in BeginConnect where there is no JS stack any more...?

Honza
Oh, hmm.  Yes, that call is now happening async, so the JS has unwound off the stack.

Is Firebug using that notification for some sort of "network load started" breakpoint?  I really hope that's not expected to work in general, since there are lots of cases where AsyncOpen() is called async itself...  :(
Assignee: general → nobody
Blocks: 797684
Component: JavaScript Engine → Networking: HTTP
Summary: Missing JSD stack frames → http-on-modify-request now called after JS that started the load is no longer on the stack
So perhaps a better question is: what information, exactly, is Firebug trying to present to the user here?
proxy discovery now happens asynchronously (necessary as this was a major source of chromehang) and http-on-modify-request needs the proxy information (bug 797684) which in turn means that homr isn't going to be able to happen synchronous with asyncopen() .. but I don't see that it was ever guaranteed to be that way in the past. http-on-modify-request is generated from a different stack via DoAuthRetry for example.

so I think the bug itself is probly WONTFIX, but I also share bz's question of what firebug needs here.
Firebug needs to break JS execution on the line, which caused the XHR.

Typically on:
request.send(null);

In order to do that it must be done synchronously at the moment 'send' is called so, JS stack trace is still available.

Honza
This feature is called 'Break on XHR' and allows the user to quickly find the code that is responsible for sending XHR requests. This is useful especially in cases where the user doesn't know the (JS) code and so, doesn't know the line number where to add a 'standard' JS breakpoint.

Does it make sense?

Honza
Sounds like we should just add a notification that XMLHttpRequest::Send itself will trigger, if that's specifically what you want.
Yes, that sounds correct. Of course, the notification should be fired synchronously so, JS stack is there.

Is there anything I could do to help to get that notification ASAP?

The feature is currently broken and it's blocking Firebug from the release.

Thanks!
Honza
Well, the most likely thing to help is writing a patch...
> Well, the most likely thing to help is writing a patch...
I see, the problem is that I don't have enough experience
with the code. Anyway, it sounds like relatively simple
thing for somebody who knows the code and as soon as there
is a patch I can quickly test using Firebug...

Honza
OK.  Do you need any more information other than "XHR starting"?  e.g. do you need access to the XHR object?  To the channel?  Something else?
Yes, I need:

1) (MUST) nsIHttpChannel object - the same object that is passed into "http-on-modify-request" observer as the |subject| argument.

2) (NICE TO HAVE) It would be also useful to have the XHR object itself (the JS object) as the second argument (not necessary if this object can be accessed through the nsIHttpChannel somehow).

Honza
Honza, can you give this a shot to see whether it does what you want?  Note that this is being dispatched _before_ the channel knows its actual listener, in case that matters.
Attachment #673389 - Flags: review?(jonas)
Er, I meant "Jan", not "Honza", of course....
Assignee: nobody → bzbarsky
Whiteboard: [firebug-p1] → [need review][firebug-p1]
Comment on attachment 673389 [details] [diff] [review]
Send an observer service notification when starting an XHR load.

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

r=me if you add a test.
Attachment #673389 - Flags: review?(jonas) → review+
Comment on attachment 673389 [details] [diff] [review]
Send an observer service notification when starting an XHR load.

One thought: if the observer cancels the xhr, what will/should happen?

thought #2: this notification seems to happen before asyncOpen(), therefore the channel will not have all the right headers yet, is that a problem?
Good questions both!  Jan?

I suppose I could move it to right after the AsyncOpen, if we're OK with it screwing up the timeout stuff... I guess the old setup did too.
(In reply to Boris Zbarsky (:bz) from comment #24)
> Er, I meant "Jan", not "Honza", of course....
Don't worry. 'Honza' and 'Jan' represent the same name in Czech (yes, it's confusing). I am used to both.

> One thought: if the observer cancels the xhr, what will/should happen?
I am not sure what are the options/consequences, but I would expect the same logic as if XHR is aborted...?

> thought #2: this notification seems to happen before asyncOpen(), therefore
> the channel will not have all the right headers yet, is that a problem?
It's not a problem for this particular case (break on XHR), but I agree that other tools could find it useful to have the headers ready.

(it could also allow to implement break-on-xhr only if specific header is set).

Thought #3: Since "xhr-load-starting" has been introduced, could we have corresponding "xhr-load-finished" event? This way it could be also simple to watch life time of a XHR. Again, nsIXMLHttpRequest would be passed to the listener as an argument (subject).

Honza
> could we have corresponding "xhr-load-finished" event?

You can do that already with readystate listeners, right?  There's a nonzero performance cost to these global observer notifications, so I'd like to keep them to a minimum.

Canceling from inside the observer working like abort would be a bit weird if the XHR is sync, since one _can't_ abort those, afaik.  So what should happen if the observer cancels a sync XHR?
(In reply to Boris Zbarsky (:bz) from comment #29)
> > could we have corresponding "xhr-load-finished" event?
> 
> You can do that already with readystate listeners, right?
Yes

> There's a nonzero
> performance cost to these global observer notifications, so I'd like to keep
> them to a minimum.
OK

> Canceling from inside the observer working like abort would be a bit weird
> if the XHR is sync, since one _can't_ abort those, afaik.  So what should
> happen if the observer cancels a sync XHR?

Does anyone have any suggestions here?

Honza
I think it would be ok to abort sync requests. I think it can technically happen if you call abort() from an event handler from the XHR object. Or if the user leaves the page (which can happen since we spin the event loop).
Any update on this one?

Jonas, do we need to update the patch?

Honza
Basically, this is a low priority for me.  I don't have the time to go through and spec out a behavior here.  If someone does the speccing out, I can probably implement, but really the patch in here should be enough for someone else to go on with implementing as needed.  But again, the first thing we need is some agreement on desired behavior, which seems to be missing here so far.

Or did we already decide on a behavior and I missed it?
(In reply to Boris Zbarsky (:bz) from comment #33)
> Basically, this is a low priority for me.  I don't have the time to go
> through and spec out a behavior here.  If someone does the speccing out, I
> can probably implement, but really the patch in here should be enough for
> someone else to go on with implementing as needed.  But again, the first
> thing we need is some agreement on desired behavior, which seems to be
> missing here so far.
> 
> Or did we already decide on a behavior and I missed it?

I can summarize the behavior from Firebug perspective:
1) A synchronous notification should be sent when XHR starts
2) The notification can be sent before asyncOpen() since Firebug doesn't need the headers
3) The nsIHttpChannel object should be passed to the handler as |subject|
4) Calling abort() from within the handler should cancel both sync and async XHRs.

---

Since Firebug is now broken (i.e. the 'break on XHR' feature) due to the change in Bug 797684, it would be great if the new API can land soon so, we (Firebug team) can fix it.

Honza
(In reply to Jan Honza Odvarko from comment #34)
> (In reply to Boris Zbarsky (:bz) from comment #33)
> > Basically, this is a low priority for me.  I don't have the time to go
> > through and spec out a behavior here.  If someone does the speccing out, I
> > can probably implement, but really the patch in here should be enough for
> > someone else to go on with implementing as needed.  But again, the first
> > thing we need is some agreement on desired behavior, which seems to be
> > missing here so far.
> > 
> > Or did we already decide on a behavior and I missed it?
> 
> I can summarize the behavior from Firebug perspective:
> 1) A synchronous notification should be sent when XHR starts
> 2) The notification can be sent before asyncOpen() since Firebug doesn't
> need the headers

These should be easily satisfied by calling the observer right before we start spinning the event loop in case of synchronous requests. I.e. here:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#3092


> 3) The nsIHttpChannel object should be passed to the handler as |subject|

Isn't it better to pass the XHR object? Since it's easy to get to the channel from the XHR object by accessing xhr.channel.

Going from the channel to the xhr object is much more awkward.

> 4) Calling abort() from within the handler should cancel both sync and async
> XHRs.

Do you mean calling xhr.abort() or calling channel.cancel()?

In any case, I think if we send the notification right before we start spinning the event loop it should work.
(In reply to Jonas Sicking (:sicking) from comment #35)
> > I can summarize the behavior from Firebug perspective:
> > 1) A synchronous notification should be sent when XHR starts
> > 2) The notification can be sent before asyncOpen() since Firebug doesn't
> > need the headers
> 
> These should be easily satisfied by calling the observer right before we
> start spinning the event loop in case of synchronous requests. I.e. here:
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/
> nsXMLHttpRequest.cpp#3092
Yes, and it's what the attached patch does.

> > 3) The nsIHttpChannel object should be passed to the handler as |subject|
> 
> Isn't it better to pass the XHR object? Since it's easy to get to the
> channel from the XHR object by accessing xhr.channel.
Good point and works for me.

> Going from the channel to the xhr object is much more awkward.
True

> > 4) Calling abort() from within the handler should cancel both sync and async
> > XHRs.
> Do you mean calling xhr.abort() or calling channel.cancel()?
Both should work if executed within the handler.

> In any case, I think if we send the notification right before we start
> spinning the event loop it should work.
Sounds good.

So, it sounds like the attached patch is good enough to land?

Honza
Jan, does this work for you?
Attachment #683144 - Flags: review?(jonas)
Attachment #673389 - Attachment is obsolete: true
Oh, and I guess I still need a test.  Working on that.
Attachment #683259 - Flags: review?(jonas)
Attachment #683144 - Attachment is obsolete: true
Attachment #683144 - Flags: review?(jonas)
Given bug 811669 comment 7 it looks like it might be easiest to add some other http-on-XXX-request call (that gets called synchronously during asyncOpen, but w/o proxy info avail), and solve both this bug and that one with it, rather than having this XHR-specific call.  Unless the convenience of having an event provide the XHR object (rather than than the necko channel) is compelling.
If you're planning to do something like that for Firefox 19 (that means on Aurora at this point), we could not do anything XHR-specific here, yes...
(In reply to Boris Zbarsky (:bz) from comment #37)
> Created attachment 683144 [details] [diff] [review]
> Send an observer service notification when starting an XHR load.
> 
> Jan, does this work for you?
Yes, I tried your patch, built Firefox, patched also the Firebug side and it works well. The related Firebug issue report is here: http://code.google.com/p/fbug/issues/detail?id=6083


(In reply to Jason Duell (:jduell) from comment #40)
> Given bug 811669 comment 7 it looks like it might be easiest to add some
> other http-on-XXX-request call (that gets called synchronously during
> asyncOpen, but w/o proxy info avail), and solve both this bug and that one
> with it, rather than having this XHR-specific call.  Unless the convenience
> of having an event provide the XHR object (rather than than the necko
> channel) is compelling.
If the new event should fall into the same category of events (i.e. "http-on-modify-request", "http-on-examine-response" and "http-on-examine-cached-response")

and perhaps being renamed to e.g. http-on-xhr-load-started" than I would also recommend to use the necko-channel as the subject. All these events are typically handled by one 'observe' method and it's slightly easier/cleaner to keep the same subject.

At the same time, having a simple API that returns XHR from nsIHttpChannel (if any) would be also useful (the underlying XHR must be currently accessed through notificationCallbacks, which is a bit annoying).

Jason, do you have any time estimate for the implementation? Firebug is currently broken and waiting impatiently for the patch ;-)

Honza
Here's a patch with a new 'http-on-opening-request' event.  I made it fire only during the initial open of a channel, not during redirects, both because that seems to be what XHR wants here and because it makes the semantics cleaner (otherwise it would be "guaranteed to be called synchronously, except when it's not").

if "mOriginalURI == mURI" isn't a reliable enough stand-in for "this is a redirect" I can add an mIsRedirect flag.  But I think this works.

If this looks good I can roll a test for it.
Attachment #683848 - Flags: review?(bzbarsky)
> if "mOriginalURI == mURI" isn't a reliable enough stand-in for "this is a redirect"

It's not, but you could instead use the actual request flag we have for flagging redirects, right?
Comment on attachment 683848 [details] [diff] [review]
Alternative: fire HTTP-level 'on-opening-request' event.

Apart from the "how to tell we're a redirect" bit, this seems fine to me.  But do fix that bit.
Attachment #683848 - Flags: review?(bzbarsky) → review+
Attached patch interdiff, v1->v2 (obsolete) — Splinter Review
bz: I assume you mean I should use LOAD_REPLACE?
Assignee: bzbarsky → jduell.mcbugs
Attached patch v2 (obsolete) — Splinter Review
Attachment #683259 - Attachment is obsolete: true
Attachment #683848 - Attachment is obsolete: true
Attachment #685294 - Flags: review?(bzbarsky)
Comment on attachment 685294 [details] [diff] [review]
v2

> bz: I assume you mean I should use LOAD_REPLACE?

Yes.

You don't need to change the IID of nsIHttpProtocolHandler.idl.  In fact, given that we need to land this on branches you really don't want to change it.

r=me
Attachment #685294 - Flags: review?(bzbarsky) → review+
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 797684 
User impact if declined:  Firebug 'Break on XHR' feature will be broken (see comment 16) 
Risk to taking this patch (and alternatives if risky): low: this just fires a new event that no existing code uses.
String or UUID changes made by this patch: none
Attachment #685293 - Attachment is obsolete: true
Attachment #685294 - Attachment is obsolete: true
Attachment #685765 - Flags: review+
Attachment #685765 - Flags: approval-mozilla-beta?
Attachment #685765 - Flags: approval-mozilla-aurora?
Jan:

So you'll need to teach firebug to listen to the 'http-on-opening-request' notification instead of http-on-modify-request for the affected versions (presumably firefox >= 18 if we get aurora/beta approval).

I have updated 
  
  https://developer.mozilla.org/en-US/docs/Observer_Notifications

keeping [dev-doc-needed] for now so we remember to update the version that's affected when we determine aurora/beta +a.  AFAIK that page (and the IDL change in the patch) is all the documentation that's needed.
Keywords: dev-doc-needed
Attachment #685765 - Flags: approval-mozilla-beta?
Attachment #685765 - Flags: approval-mozilla-beta+
Attachment #685765 - Flags: approval-mozilla-aurora?
Attachment #685765 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/3c3a8eed0578
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Updated 

  https://developer.mozilla.org/en-US/docs/Observer_Notifications
  
to note that this was introduced in FF18, which is the end of the documentation needed AFAIK.
Keywords: dev-doc-needed
Tested with Firebug and works great, thanks!
Honza
Status: RESOLVED → VERIFIED
In the past "http-on-modify-request" could be used to filter URLs. This worked great for all URLs, even for redirects. It is important that it also works for redirects, because some sites use their own URLs to redirect to other unwanted domains (HTTP 30x codes).

If I understand it right, "http-on-modify-request" doesn't work for filtering any longer. It isn't called in "HttpChannelChild::AsyncOpen" anymore. Instead "http-on-opening-request" is called in "HttpChannelChild::AsyncOpen" now. So this function could provide the old functionality. But it is important that you call it for all URLs. Don't skip redirects, please! If you think some functions don't want to be notified about redirects, why don't you add a flag that signals redirects.

Or am I wrong and there is another way to filter URLs? I couldn't find one.

The only alternative I can think of is a proxy filter for DNS and content. But that's much more complicated.
http-on-modify-request should still work fine for the filtering thing, no?  It just didn't work for Firebug's specific "JS that started the load must still be on the stack" use case.
Yes, on-modify-request is still called before the network request is made, and it should work as always unless your caller relies on it being called *while* asyncOpen is being called.  That shouldn't be the case for most callers.  

Let me know if you run into use cases where it doesn't work for you.
Just as a heads up, this change to the notification of http-on-modify-request events to extensions affected me.

My use case is that I use http-on-modify-request to look at requests going out, I look at their URIs and then for requests that are of interest to me I modify the POST data. I do this by QIing the http channel and then calling setUploadStream with a nsIStringInputStream wrapped around my modified POST body, in a function called from my observe function on http-on-modify-request. 

Lately the setUploadStream call was giving me a NS_ERROR_IN_PROGRESS- now I know why! I guess that modifying the POST data of a request in progress is a use case where I need my handler called during asyncOpen. 

Anyway I have changed my extension to listen to http-on-opening-request instead and it's working as intended now with no other changes, so just posting this FYI and for search engines.
> Lately the setUploadStream call was giving me a NS_ERROR_IN_PROGRESS- now I know
> why! I guess that modifying the POST data of a request in progress is a use case 
> where I need my handler called during asyncOpen. 

This is entirely my fault, and I apologize.  The change that broke your code was bug 797684 (which landed on firefox 18, the curent release version of Firefox).  Bug 811669 (currently landed on mozilla 19, i.e. beta) fixed it, but I didn't get a backport to the 18 branch in time, and now this bustage is out in the world for six weeks. As of Firefox 19 (due to ship at the end of February) your original on-modify-request code should work fine again (you should be able to verify this by trying to run it on a Firefox beta build).

Brown paper bag over the head day for me :(
Don't worry about it, it's my fault for not keeping track of things and testing my extension on beta before Firefox releases.

Anyway, yup, I tested on beta and the on-modify-request code works as expected. So to update my extension I just had it figure out if it was on Firefox 18 or 'some other version not 18' and register for on-opening-request if the former and on-modify-request if the latter.

if(versionChecker.compare(appInfo.version, "18") >= 0 && versionChecker.compare(appInfo.version, "19") < 0) {
observerService.addObserver(obj, "http-on-opening-request", false);
}else{
observerService.addObserver(obj, "http-on-modify-request", false);
}

and now the same XPI works on stable and beta. Thanks for you comments Jason, as an extension developer I really appreciate how open you guys are and how much can be achieved in an extension that is just not possible in other browsers.
Mozilla/5.0 (Windows NT 6.0; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0

Verified on Firefox 19 beta 6 (buildID: 20130212082553), latest Nightly (buildID: 20130212031120) and latest Aurora (buildID: 20130212042017).
User Agent: Mozilla/5.0 (Windows NT 6.0; rv:20.0) Gecko/20100101 Firefox/20.0

Verified as fixed on Firefox 20 beta 7 (Build ID: 20130325214615).
You need to log in before you can comment on or make changes to this bug.