Last Comment Bug 800799 - http-on-modify-request now called after JS that started the load is no longer on the stack
: http-on-modify-request now called after JS that started the load is no longer...
Status: VERIFIED FIXED
[need review][firebug-p1]
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Trunk
: x86 Windows Vista
: -- normal with 1 vote (vote)
: mozilla20
Assigned To: Jason Duell [:jduell] (needinfo? me)
:
Mentors:
: 802551 (view as bug list)
Depends on:
Blocks: 797684
  Show dependency treegraph
 
Reported: 2012-10-11 23:38 PDT by Jan Honza Odvarko [:Honza]
Modified: 2016-02-17 17:31 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
fixed
verified
verified


Attachments
Send an observer service notification when starting an XHR load. (1.63 KB, patch)
2012-10-19 13:14 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
jonas: review+
Details | Diff | Review
Send an observer service notification when starting an XHR load. (1.43 KB, patch)
2012-11-19 08:41 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review
Now with tests and a bogus-assert fix (4.84 KB, patch)
2012-11-19 12:07 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
jonas: review+
Details | Diff | Review
Alternative: fire HTTP-level 'on-opening-request' event. (3.75 KB, patch)
2012-11-20 19:14 PST, Jason Duell [:jduell] (needinfo? me)
bzbarsky: review+
Details | Diff | Review
interdiff, v1->v2 (757 bytes, patch)
2012-11-26 12:50 PST, Jason Duell [:jduell] (needinfo? me)
no flags Details | Diff | Review
v2 (4.40 KB, patch)
2012-11-26 12:51 PST, Jason Duell [:jduell] (needinfo? me)
bzbarsky: review+
Details | Diff | Review
v2. Version landed on m-c (no IDL version upgrade) (3.64 KB, patch)
2012-11-27 12:05 PST, Jason Duell [:jduell] (needinfo? me)
jduell.mcbugs: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review

Description Jan Honza Odvarko [:Honza] 2012-10-11 23:38:23 PDT
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
Comment 1 Luke Wagner [:luke] 2012-10-12 08:53:50 PDT
How about FF18?  IonMonkey came with some changes to the debugger's view of the stack (viz., JS_BrokenFrameIterator).
Comment 2 Nicolas B. Pierron [:nbp] 2012-10-12 11:48:43 PDT
(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.
Comment 3 Jan Honza Odvarko [:Honza] 2012-10-15 06:07:17 PDT
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
Comment 4 Nicolas B. Pierron [:nbp] 2012-10-17 10:44:59 PDT
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)
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-17 11:02:36 PDT
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?
Comment 6 Sebastian Zartner [:sebo] 2012-10-17 14:54:36 PDT
> 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
Comment 7 Nicolas B. Pierron [:nbp] 2012-10-17 16:32:30 PDT
(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?
Comment 8 Jan Honza Odvarko [:Honza] 2012-10-18 04:36:34 PDT
(In reply to Boris Zbarsky (:bz) from comment #5)
> Jan, can you find a one-day regression range for this?

Works:
Built from http://hg.mozilla.org/mozilla-central/rev/aa5e3b445810
(downloaded from: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-10-09-03-05-47-mozilla-central/)

Does not work:
Built from http://hg.mozilla.org/mozilla-central/rev/ec10630b1a54
(downloaded from: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-10-10-03-06-05-mozilla-central/)

Honza
Comment 9 Jan Honza Odvarko [:Honza] 2012-10-18 04:45:59 PDT
(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
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-18 06:21:25 PDT
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...
Comment 11 Jan Honza Odvarko [:Honza] 2012-10-18 06:43:05 PDT
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
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-18 06:47:37 PDT
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...  :(
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-18 06:48:37 PDT
So perhaps a better question is: what information, exactly, is Firebug trying to present to the user here?
Comment 14 Patrick McManus [:mcmanus] 2012-10-18 07:00:26 PDT
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.
Comment 15 Jan Honza Odvarko [:Honza] 2012-10-18 07:47:07 PDT
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
Comment 16 Jan Honza Odvarko [:Honza] 2012-10-18 07:53:06 PDT
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
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-18 07:55:35 PDT
Sounds like we should just add a notification that XMLHttpRequest::Send itself will trigger, if that's specifically what you want.
Comment 18 Jan Honza Odvarko [:Honza] 2012-10-18 07:59:56 PDT
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
Comment 19 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-18 08:04:38 PDT
Well, the most likely thing to help is writing a patch...
Comment 20 Jan Honza Odvarko [:Honza] 2012-10-18 08:15:55 PDT
> 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
Comment 21 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-18 08:31:47 PDT
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?
Comment 22 Jan Honza Odvarko [:Honza] 2012-10-18 08:41:26 PDT
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
Comment 23 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-19 13:14:52 PDT
Created attachment 673389 [details] [diff] [review]
Send an observer service notification when starting an XHR load.

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.
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-19 13:18:07 PDT
Er, I meant "Jan", not "Honza", of course....
Comment 25 Jonas Sicking (:sicking) PTO Until July 5th 2012-10-19 14:45:35 PDT
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.
Comment 26 Christian :Biesinger (don't email me, ping me on IRC) 2012-10-19 16:11:45 PDT
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?
Comment 27 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-19 17:45:18 PDT
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.
Comment 28 Jan Honza Odvarko [:Honza] 2012-10-21 23:37:58 PDT
(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
Comment 29 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-21 23:56:52 PDT
> 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?
Comment 30 Jan Honza Odvarko [:Honza] 2012-10-22 08:43:41 PDT
(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
Comment 31 Jonas Sicking (:sicking) PTO Until July 5th 2012-10-22 12:12:14 PDT
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).
Comment 32 Jan Honza Odvarko [:Honza] 2012-11-01 04:57:36 PDT
Any update on this one?

Jonas, do we need to update the patch?

Honza
Comment 33 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-11-01 09:04:29 PDT
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?
Comment 34 Jan Honza Odvarko [:Honza] 2012-11-02 04:52:29 PDT
(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
Comment 35 Jonas Sicking (:sicking) PTO Until July 5th 2012-11-19 01:12:39 PST
(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.
Comment 36 Jan Honza Odvarko [:Honza] 2012-11-19 03:35:12 PST
(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
Comment 37 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-11-19 08:41:48 PST
Created attachment 683144 [details] [diff] [review]
Send an observer service notification when starting an XHR load.

Jan, does this work for you?
Comment 38 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-11-19 08:42:29 PST
Oh, and I guess I still need a test.  Working on that.
Comment 39 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-11-19 12:07:30 PST
Created attachment 683259 [details] [diff] [review]
Now with tests and a bogus-assert fix
Comment 40 Jason Duell [:jduell] (needinfo? me) 2012-11-19 15:38:28 PST
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.
Comment 41 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-11-19 18:09:20 PST
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...
Comment 42 Jan Honza Odvarko [:Honza] 2012-11-20 04:39:59 PST
(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
Comment 43 Jason Duell [:jduell] (needinfo? me) 2012-11-20 19:14:16 PST
Created attachment 683848 [details] [diff] [review]
Alternative: fire HTTP-level 'on-opening-request' event.

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.
Comment 44 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-11-20 19:50:58 PST
> 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 45 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-11-20 19:52:47 PST
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.
Comment 46 Jason Duell [:jduell] (needinfo? me) 2012-11-26 12:50:02 PST
Created attachment 685293 [details] [diff] [review]
interdiff, v1->v2

bz: I assume you mean I should use LOAD_REPLACE?
Comment 47 Jason Duell [:jduell] (needinfo? me) 2012-11-26 12:51:05 PST
Created attachment 685294 [details] [diff] [review]
v2
Comment 48 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-11-26 18:44:24 PST
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
Comment 49 Jason Duell [:jduell] (needinfo? me) 2012-11-27 12:05:49 PST
Created attachment 685765 [details] [diff] [review]
v2. Version landed on m-c (no IDL version upgrade)

[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
Comment 50 Jason Duell [:jduell] (needinfo? me) 2012-11-27 12:06:27 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c3a8eed0578
Comment 51 Jason Duell [:jduell] (needinfo? me) 2012-11-27 12:21:47 PST
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.
Comment 52 Ryan VanderMeulen [:RyanVM] 2012-11-27 16:22:39 PST
https://hg.mozilla.org/mozilla-central/rev/3c3a8eed0578
Comment 54 Jason Duell [:jduell] (needinfo? me) 2012-11-27 17:16:45 PST
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.
Comment 55 Jason Duell [:jduell] (needinfo? me) 2012-11-27 17:30:24 PST
Also updated 

   https://developer.mozilla.org/en-US/docs/Firefox_18_for_developers
Comment 56 Jan Honza Odvarko [:Honza] 2012-11-29 03:16:15 PST
Tested with Firebug and works great, thanks!
Honza
Comment 57 bugzilla2002 2012-12-12 16:36:58 PST
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.
Comment 58 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-12-12 18:05:44 PST
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.
Comment 59 Jason Duell [:jduell] (needinfo? me) 2012-12-13 10:09:32 PST
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.
Comment 60 Karl Reid 2013-01-29 17:17:03 PST
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.
Comment 61 Jason Duell [:jduell] (needinfo? me) 2013-01-29 20:51:35 PST
> 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 :(
Comment 62 Karl Reid 2013-01-30 08:47:17 PST
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.
Comment 63 Bogdan Maris, QA [:bogdan_maris] 2013-02-13 05:53:39 PST
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).
Comment 64 Alexandra Lucinet, QA Mentor [:adalucinet][PTO: 21st June - 5th July] 2013-03-26 08:31:52 PDT
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).
Comment 65 Patrick McManus [:mcmanus] 2016-02-17 10:00:05 PST
*** Bug 802551 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.