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)
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)
3.64 KB,
patch
|
jduell.mcbugs
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
How about FF18? IonMonkey came with some changes to the debugger's view of the stack (viz., JS_BrokenFrameIterator).
Comment 2•12 years ago
|
||
(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.
Reporter | ||
Comment 3•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
> 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•12 years ago
|
||
(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?
Reporter | ||
Comment 8•12 years ago
|
||
(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
Reporter | ||
Comment 9•12 years ago
|
||
(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•12 years ago
|
||
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...
Reporter | ||
Comment 11•12 years ago
|
||
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•12 years ago
|
||
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... :(
Updated•12 years ago
|
Summary: Missing JSD stack frames → http-on-modify-request now called after JS that started the load is no longer on the stack
Comment 13•12 years ago
|
||
So perhaps a better question is: what information, exactly, is Firebug trying to present to the user here?
Comment 14•12 years ago
|
||
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.
Reporter | ||
Comment 15•12 years ago
|
||
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
Reporter | ||
Comment 16•12 years ago
|
||
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•12 years ago
|
||
Sounds like we should just add a notification that XMLHttpRequest::Send itself will trigger, if that's specifically what you want.
Reporter | ||
Comment 18•12 years ago
|
||
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•12 years ago
|
||
Well, the most likely thing to help is writing a patch...
Reporter | ||
Comment 20•12 years ago
|
||
> 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•12 years ago
|
||
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?
Reporter | ||
Comment 22•12 years ago
|
||
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•12 years ago
|
||
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)
Updated•12 years ago
|
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 26•12 years ago
|
||
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•12 years ago
|
||
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.
Reporter | ||
Comment 28•12 years ago
|
||
(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•12 years ago
|
||
> 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?
Reporter | ||
Comment 30•12 years ago
|
||
(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).
Reporter | ||
Comment 32•12 years ago
|
||
Any update on this one?
Jonas, do we need to update the patch?
Honza
Comment 33•12 years ago
|
||
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?
Reporter | ||
Comment 34•12 years ago
|
||
(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.
Reporter | ||
Comment 36•12 years ago
|
||
(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
Updated•12 years ago
|
Attachment #673389 -
Attachment is obsolete: true
Comment 38•12 years ago
|
||
Oh, and I guess I still need a test. Working on that.
Comment 39•12 years ago
|
||
Attachment #683259 -
Flags: review?(jonas)
Updated•12 years ago
|
Attachment #683144 -
Attachment is obsolete: true
Attachment #683144 -
Flags: review?(jonas)
Assignee | ||
Comment 40•12 years ago
|
||
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•12 years ago
|
||
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...
Reporter | ||
Comment 42•12 years ago
|
||
(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
Attachment #683259 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 43•12 years ago
|
||
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)
Comment 44•12 years ago
|
||
> 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•12 years ago
|
||
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+
Assignee | ||
Comment 46•12 years ago
|
||
bz: I assume you mean I should use LOAD_REPLACE?
Assignee: bzbarsky → jduell.mcbugs
Assignee | ||
Comment 47•12 years ago
|
||
Attachment #683259 -
Attachment is obsolete: true
Attachment #683848 -
Attachment is obsolete: true
Attachment #685294 -
Flags: review?(bzbarsky)
Comment 48•12 years ago
|
||
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+
Assignee | ||
Comment 49•12 years ago
|
||
[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?
Assignee | ||
Comment 50•12 years ago
|
||
Assignee | ||
Comment 51•12 years ago
|
||
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
Updated•12 years ago
|
Attachment #685765 -
Flags: approval-mozilla-beta?
Attachment #685765 -
Flags: approval-mozilla-beta+
Attachment #685765 -
Flags: approval-mozilla-aurora?
Attachment #685765 -
Flags: approval-mozilla-aurora+
Comment 52•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Assignee | ||
Comment 53•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/b8c7f8010cca
https://hg.mozilla.org/releases/mozilla-beta/rev/c3a1545d4685
status-firefox17:
--- → unaffected
status-firefox18:
--- → fixed
status-firefox19:
--- → fixed
status-firefox20:
--- → fixed
Assignee | ||
Comment 54•12 years ago
|
||
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
Assignee | ||
Comment 55•12 years ago
|
||
Reporter | ||
Comment 56•12 years ago
|
||
Tested with Firebug and works great, thanks!
Honza
Status: RESOLVED → VERIFIED
Comment 57•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee | ||
Comment 59•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee | ||
Comment 61•12 years ago
|
||
> 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•12 years ago
|
||
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•12 years ago
|
||
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).
Updated•12 years ago
|
Comment 64•12 years ago
|
||
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.
Description
•