Closed
Bug 924905
Opened 11 years ago
Closed 11 years ago
Missing stack frames coming from the content scope in a chrome event listener attached to a content node
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla29
People
(Reporter: Honza, Assigned: luke)
References
Details
(Keywords: regression, Whiteboard: [firebug-p1])
Attachments
(4 files, 4 obsolete files)
(deleted),
application/x-xpinstall
|
Details | |
(deleted),
patch
|
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
application/x-xpinstall
|
Details | |
(deleted),
patch
|
luke
:
review+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Firebug is handling mutation events and using them for a feature called: break on mutation event.
It works as follows:
1) A mutation event is fired.
2) Firebug handler is executed
3) Firebug executes "debugger;"
4) JSD onDebugger hook is executed.
5) Firebug gets the most recent content stack frame (peeling off the ones coming from the chrome scope) and shows it to the user in the Script panel.
This doesn't work in Nightly since there are only the chrome stack frames. The frames coming from the content scope (the page) are missing.
STR:
1) Install the attached extension
2) Run Firefox and load: https://getfirebug.com/tests/head/html/breakpoints/breakOnElement.html
3) Right click on the page and pick "Hook Attribute Mutation" (the menu is coming from the test extension)
4) Click on the "Break on Attribute Modified" button.
5) Now you need to watch logs...
Watch the system console (logs done through dump) or error-console (logs done through Components.utils.reportError)
- Stack frames are logged
- It works in Aurora (you can see content stack frames), but not in Nightly (there are only chrome stack frames)
Source of the extension is here:
https://github.com/firebug/manual-tests/tree/master/mozilla/break-on-mutation
Honza
Reporter | ||
Comment 1•11 years ago
|
||
Btw. the extension also uses Components.stack to get the frames and it's also broken.
Honza
Comment 2•11 years ago
|
||
Regression range would be great. Sounds like something which changes to xpconnect may have had caused.
Updated•11 years ago
|
Flags: needinfo?(odvarko)
Keywords: regression,
regressionwindow-wanted
Reporter | ||
Comment 3•11 years ago
|
||
Tested on mozilla-central nightly builds (Win32)
Works: Built from http://hg.mozilla.org/mozilla-central/rev/f97307cb4c95
Doesn't work: from http://hg.mozilla.org/mozilla-central/rev/68d279364a8b
Honza
Flags: needinfo?(odvarko)
Comment 4•11 years ago
|
||
In that range, a plausible "culprit" is bug 862627, which might have changed which JSContexts things run in. Let me double-check that.
Comment 5•11 years ago
|
||
> 3) Right click on the page and pick "Hook Attribute Mutation"
When I do that, I get a JS error in the console:
JavaScript error: chrome://simpledebugger/content/debugger.js, line 117: NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.import]
That line of JS is:
Cu["import"]("resource://fbtrace/firebug-trace-service.js");
Does this extension rely on Firebug also being installed?
Comment 6•11 years ago
|
||
In any case, looking at the testcase extension, it's chrome code adding an event listener to a content node.
We used to run that event listener on the JSContext of the event target (so of the web page).
Now, after bug 862627, we run it on the JSContext of the listener's global (so chrome).
This fixes a bug we had wherein an exception in the listener would get reported to the page's onerror instead of the chrome window's. On the other hand it also means that the JS stacks, which are per-JSContext, now have nothing to do with each other.
The right solution here is to stop relying on these arbitrary JSContext distinctions for stacks or onerror or both... The question is what the right thing for Firefox 27 is. Can we just give Firebug a simple API to examine the current stack on the JSContext associated with the event target's global or something?
Reporter | ||
Comment 7•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] (Vacation Oct 12 - Oct 27) from comment #5)
> > 3) Right click on the page and pick "Hook Attribute Mutation"
>
> When I do that, I get a JS error in the console:
>
> JavaScript error: chrome://simpledebugger/content/debugger.js, line 117:
> NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057
> (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.import]
>
> That line of JS is:
>
> Cu["import"]("resource://fbtrace/firebug-trace-service.js");
>
> Does this extension rely on Firebug also being installed?
No, it should not depend on Firebug.
Fixed and new version of the extension attached.
Honza
Attachment #814878 -
Attachment is obsolete: true
Updated•11 years ago
|
Flags: needinfo?(luke)
Flags: needinfo?(bobbyholley+bmo)
Updated•11 years ago
|
Summary: Missing stack frames coming from the content scope → Missing stack frames coming from the content scope in a chrome event listener attached to a content node
Reporter | ||
Comment 8•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] (Vacation Oct 12 - Oct 27) from comment #6)
> In any case, looking at the testcase extension, it's chrome code adding an
> event listener to a content node.
>
> We used to run that event listener on the JSContext of the event target (so
> of the web page).
>
> Now, after bug 862627, we run it on the JSContext of the listener's global
> (so chrome).
>
> This fixes a bug we had wherein an exception in the listener would get
> reported to the page's onerror instead of the chrome window's. On the other
> hand it also means that the JS stacks, which are per-JSContext, now have
> nothing to do with each other.
>
> The right solution here is to stop relying on these arbitrary JSContext
> distinctions for stacks or onerror or both... The question is what the
> right thing for Firefox 27 is. Can we just give Firebug a simple API to
> examine the current stack on the JSContext associated with the event
> target's global or something?
Yes, that sounds good.
All what Firebug needs is to get the web page most recent stack frame and
point the Script panel UI (the debugger) to it, so the user can see where
the mutation event originated. In other words, Firebug needs reference to
the most recent |frame| object coming from the page.
Firebug is not even interested in chrome stacks so, having the JSContext
(i.e. the stack) of the page directly sounds like even better API.
Honza
Assignee | ||
Comment 9•11 years ago
|
||
I'm not familiar with all the magic associated with a JSContext by the DOM, but it seems like we wouldn't need to switch JSContexts, but rather just change compartments when calling the chrome event listener. This is the direction we want to go (for JSContext-removal); perhaps we could move some of the JSContext magic to compartments?
That being said, ultimately, each JSRuntime does maintain a single stack on which all JSContexts interleave. By default, stack iteration filters to a single JSContext, but it's easy enough to turn off that filter (via AllFramesIter). AllFramesIter is JS-engine-internal, though, so to benefit from this we'd need to see which public API Firebug is using and fix that.
Flags: needinfo?(luke)
Comment 10•11 years ago
|
||
> I'm not familiar with all the magic associated with a JSContext by the DOM,
It's sort of associated by the JS engine, actually: the error reporter is per-JSContext, so the only way to associate a JSErrorReport with a global (needed for firing onerror events) is via the JSContext it's reported on, as far as I can see.
> it seems like we wouldn't need to switch JSContexts, but rather just change compartments
> when calling the chrome event listener.
That's what we used to do. It's really complicated and slow to implement in practice (e.g. you have to dig up "the current JSContext I'm running on"), and lead to errors being reported on the wrong globals, as I said above.
> This is the direction we want to go
Sure, after we remove all state from the JSContext so it becomes functionally equivalent to a JSRuntime. But we're not going to get there in 27, I suspect.
> each JSRuntime does maintain a single stack
Oh, interesting! Did that always use to be the case?
> so to benefit from this we'd need to see which public API Firebug is using and fix that.
The attached testcase is doing:
for (var frame = Components.stack; frame; frame = frame.caller)
which lands us at JSStackFrame::CreateStack, which calls JS::DescribeStack.
Reporter | ||
Comment 11•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] (Vacation Oct 12 - Oct 27) from comment #10)
> The attached testcase is doing:
>
> for (var frame = Components.stack; frame; frame = frame.caller)
Just to note that the test case uses two ways to get the list of stack frames (to show that both are failing):
1) JSD:
while (frame)
frame = frame.callingFrame;
2) Components:
for (var frame = Components.stack; frame; frame = frame.caller)
Honza
Assignee | ||
Comment 12•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] (Vacation Oct 12 - Oct 27) from comment #10)
> > it seems like we wouldn't need to switch JSContexts, but rather just change compartments
> > when calling the chrome event listener.
>
> That's what we used to do. It's really complicated and slow to implement in
> practice (e.g. you have to dig up "the current JSContext I'm running on"),
> and lead to errors being reported on the wrong globals, as I said above.
Well, by "change compartments" I meant "using the JSContext you just have on hand), but it sounds like, even if we fix error reporting to be based on the current compartment, we'd still have other per-JSContext state that makes JSContexts distinguishable.
(In reply to Jan Honza Odvarko from comment #11)
> (In reply to Boris Zbarsky [:bz] (Vacation Oct 12 - Oct 27) from comment #10)
> > The attached testcase is doing:
> >
> > for (var frame = Components.stack; frame; frame = frame.caller)
>
> Just to note that the test case uses two ways to get the list of stack
> frames (to show that both are failing):
>
> 1) JSD:
> while (frame)
> frame = frame.callingFrame;
>
> 2) Components:
> for (var frame = Components.stack; frame; frame = frame.caller)
Do you know what .callingFrame ends up calling in the JS engine?
Fixing JS::DescribeStack isn't hard, though. The only thing is that now you'll get everything that's running, always (running on content will see chrome, content could see unrelated content, etc). It seems like there is only one caller (JSStackFrame::CreateStack); I don't know if it is used in any security-sensitive situations.
Comment 13•11 years ago
|
||
> using the JSContext you just have on hand
I don't have one on hand. I'm just C++ code calling a C++ DispatchEvent function; the fact that JS is involved at all is an implementation detail of that DispatchEvent function.
> Do you know what .callingFrame ends up calling in the JS engine?
jsd_GetCallingStackFrame. It looks to me like it maintains its own stack in the JSDThreadState or something?
As far as where it gets that stack, looks like JSBrokenFrameIterator (in jsd_NewThreadState) maybe?
Assignee | ||
Comment 14•11 years ago
|
||
Alright, JSBrokenFrameIterator just stores and uses a NonBuiltinScriptFrameIter, so we can do the same as for DescribeStack. This has the same question though: is jsd expecting not to see, e.g., chrome from content and content A from content B?
Comment 15•11 years ago
|
||
_That_ is a question I don't have an answer to. :(
Comment 16•11 years ago
|
||
Yeah, I think we need Jan to answer that.
Flags: needinfo?(bobbyholley+bmo) → needinfo?(odvarko)
Reporter | ||
Comment 17•11 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #12)
> Do you know what .callingFrame ends up calling in the JS engine?
If I run the STR (comment #0) on current broken Nightly I see (in the system console, dump must be enabled):
chrome://simpledebugger/content/debugger.js@177
chrome://simpledebugger/content/debugger.js@101
null@0
If I run on Built from http://hg.mozilla.org/mozilla-central/rev/f97307cb4c95, I see:
(in this case I can see frames from the content)
chrome://simpledebugger/content/debugger.js@101
https://getfirebug.com/tests/head/html/breakpoints/breakOnElement.html@45
https://getfirebug.com/tests/head/html/breakpoints/breakOnElement.html@1
null@0
Not sure what is the oldest frame though. The: null@0
(In reply to Luke Wagner [:luke] from comment #14)
> Alright, JSBrokenFrameIterator just stores and uses a
> NonBuiltinScriptFrameIter, so we can do the same as for DescribeStack. This
> has the same question though: is jsd expecting not to see, e.g., chrome from
> content and content A from content B?
JSD is expecting to see the frames coming from the content even if the mutation observer handler comes from chrome. Till now JSD was seeing the full stack-trace, i.e. frames coming from the chrome (e.g. Firebug frames) and frames coming from the content.
Honza
Flags: needinfo?(odvarko)
Assignee | ||
Comment 18•11 years ago
|
||
Since the stack we'd expose is *all* JS running on the entire main thread, we'd also (theoretically, I don't know if this type of interleaving occurs in practice) see frames from unrelated content origins.
Comment 19•11 years ago
|
||
Would that include totally unrelated JS that might have started a sync XHR and is currently spinning an event loop? We set aside the frame chain in that case, but it sounds like that wouldn't be sufficient...
Assignee | ||
Comment 20•11 years ago
|
||
You're right, that is a good example. Fortunately, the saved-ness is recorded in the Activation (which is in the JSRuntime's stack) and ScriptFrameIter (which AllFramesIter is a trivial use of) provides a STOP_AT_SAVED option. So, JS_SaveFrameChain could be respected.
Is there anything else? In particular, AllFramesIter always starts at the innermost activation of *any* JSContext; do we always wish to iterate the stack of the innermost running JSContext?
Comment 21•11 years ago
|
||
We should always be calling JS_SaveFrameChain when we switch JSContexts, except in the funny case where we detect that the principal we would get from the default compartment object is the same as the principal of the current compartment of the cx we're switching to.
Comment 22•11 years ago
|
||
Naveed can you get an assignee on this?
Assignee | ||
Comment 23•11 years ago
|
||
Assuming the fix is as simple as what we've been discussing, I can take this.
Assignee: nobody → luke
Flags: needinfo?(nihsanullah)
Assignee | ||
Comment 24•11 years ago
|
||
So I first did what we talked about and it didn't help; the callstack only contained chrome. On a hunch, I changed the stack iterator ignore JS_SaveFrameChain calls, and I get what I think is the desired stack:
onMutateAttr; chrome://simpledebugger/content/debugger.js@177
chrome://simpledebugger/content/debugger.js@101
https://getfirebug.com/tests/head/html/breakpoints/breakOnElement.html@45
https://getfirebug.com/tests/head/html/breakpoints/breakOnElement.html@1
null@0
Assuming this is what we want, that means, to fix this, we need my patch and one in XPConnect/DOM to remove the JS_SaveFrameChain call that is severing content and chrome in this testcase. Any thoughts on this bholley?
Attached is the patch which stops at JS_SaveFrameChain; just s/STOP_AT_SAVED/GO_THROUGH_SAVED/ to see the version that includes chrome+content.
Flags: needinfo?(bobbyholley+bmo)
Comment 25•11 years ago
|
||
Going through saved frame chains is wrong, in general, since it will also show stacks through things that spin the event loop, right?
What's really wanted here is something that will show the parts of the stack that are associated with the given origin or something....
Assignee | ||
Comment 26•11 years ago
|
||
(In reply to On vacation Oct 12 - Oct 27 from comment #25)
> Going through saved frame chains is wrong, in general, since it will also
> show stacks through things that spin the event loop, right?
In general, yes, but I was wondering if, in the particular case we have here we were able to avoid the JS_SaveFrameChain. I was assuming that, before the original change that broke this, that's what we were doing. I suppose, though, before we could have had two JSContexts, one with the chrome, one with the content, and, even though the chrome's JSContext was the most recent activation, we were walking the callstack of content's JSContext. Do you know?
> What's really wanted here is something that will show the parts of the stack
> that are associated with the given origin or something....
I can definitely see an iterator which takes a JSPrincipals and skips over any frame whose JSPrincipals (as judged by the frame's scope chain's compartment's principal) isn't subsume()d by the given JSPrincipals. Do you know if we have the relevant JSPrincipal at the relevant callers of JSBrokenFrameIterator/DescriptStack?
Comment 27•11 years ago
|
||
Probably not right now; we'd have to add that.
Comment 28•11 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #26)
> In general, yes, but I was wondering if, in the particular case we have here
> we were able to avoid the JS_SaveFrameChain.
We need a JS_SaveFrameChain every time we switch JSContexts.
> I was assuming that, before
> the original change that broke this, that's what we were doing. I suppose,
> though, before we could have had two JSContexts, one with the chrome, one
> with the content, and, even though the chrome's JSContext was the most
> recent activation, we were walking the callstack of content's JSContext. Do
> you know?
I would guess that previously we were running both on the same JSContext, and the JS_SaveFrameChain call was optimized out (which we do if you push a JSContext check which is already stack-top for which invoking JS_SaveFrameChain would not alter the subject principal).
> I can definitely see an iterator which takes a JSPrincipals and skips over
> any frame whose JSPrincipals (as judged by the frame's scope chain's
> compartment's principal) isn't subsume()d by the given JSPrincipals. Do you
> know if we have the relevant JSPrincipal at the relevant callers of
> JSBrokenFrameIterator/DescriptStack?
The issue here is that we'd need to cross JS_SaveFrameChain boundaries, because we have one here (by virtue of having changed the JSContext).
I just spent some time talking with Hixie, Domenic (Promise spec editor), and gsnedders (Opera guy) about this in #whatwg. It turns out that Error.stack isn't specced (yet), so we don't really have any guidance from that angle. Domenic believes that we should show as many stack frames as possible, even through nested event loops (assuming the script is same-origin). gsnedders cautions us that the Closure library (used heavily across Google properties) does crazy per-UA processing of Error.stack, which has been a major pain for Opera. He says that the WebKit codepath would break if they started including more stack frames, but that the Gecko codepath would probably be ok. Hixie is willing to do whatever.
If we can get away with it, I think that stack frame iteration should be JSContext agnostic (thus ignoring JS_SaveFrameChain boundaries), and do a subsumes check between the subject principal and each frame before deciding whether to display it. This is what we concluded in a thread a while back, i think ("Exceptions in a Post-JSContext World").
I think we should try to do the above, but it's risky, because it will cause stack to cross nested event loop boundaries in some cases. If it breaks stuff, we have two options:
(1) Annotate the JS_SaveFrameChain calls for nested event loops to be stronger somehow. One way to do this would be to record such a boundary whenever we push a null JSContext (which we do during event processing), since that generally signals a stronger divorce between the callstacks.
(2) Make these changes apply only to the JSD API, but not Error().stack/Components.stack. We then fix up Firebug to make exclusive use of the JSD API for the time being.
I think (1) is probably best. Other thoughts?
Updated•11 years ago
|
Flags: needinfo?(bobbyholley+bmo)
Assignee | ||
Comment 29•11 years ago
|
||
To wit, the changes in the attached patch only affect JSD and Components.stack, not Error().stack.
Doing what you proposed is quite easy on our end, the main question is just which JSPrincipal to use. Both JS::DescribeStack and JSBrokenStackIterator take a JSContext*; would cx->compartment()->principal() be the right JSPrincipal to use in all the calls you can see?
Comment 30•11 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #29)
> To wit, the changes in the attached patch only affect JSD and
> Components.stack, not Error().stack.
Well, altering JS::DescribeStack will also alter DOMError().stack (see dom/bindings.Exceptions.cpp). So we should probably change Error to match simultaneously.
> Doing what you proposed is quite easy on our end, the main question is just
> which JSPrincipal to use. Both JS::DescribeStack and JSBrokenStackIterator
> take a JSContext*; would cx->compartment()->principal() be the right
> JSPrincipal to use in all the calls you can see?
I _think_ so. I can check more thoroughly when I review the eventual patch.
Comment 31•11 years ago
|
||
(though, didn't we get rid of the subsumes callback in nsJSPrincipals?)
Comment 32•11 years ago
|
||
Luke - ping? Given that the fix for this may be nontrivial, I'm pretty concerned about it slipping through the cracks until beta.
Flags: needinfo?(luke)
Assignee | ||
Comment 33•11 years ago
|
||
Isn't there a more short-term fix? IIUC, this is a recent regression. Changing all stack iteration to observe all frames subsumed by the observer's origin isn't exactly a low-risk fix.
Flags: needinfo?(luke)
Comment 34•11 years ago
|
||
We could try backing out bug 862627 but that's pretty nontrivial as well at this point.
A short-term fix seems like a new API, restricted to chrome, that Firebug can use for now. That can be a lot simpler, since it would just show all stack frames period.
Assignee | ||
Comment 35•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #34)
> A short-term fix seems like a new API, restricted to chrome, that Firebug
> can use for now. That can be a lot simpler, since it would just show all
> stack frames period.
Would either of JS::DescribeStack or JSBrokenFrameIterator satisfy that requirement? (That is, can we give one of them the magic new ability rather than introducing a 3rd new API?)
Comment 37•11 years ago
|
||
It looks like JSD is the only real consumer of JSBrokenFrameIterator, so let's just fix that.
Flags: needinfo?(bobbyholley+bmo)
Assignee | ||
Comment 39•11 years ago
|
||
It seems the 'subsumes' hook has been removed from the JSSecurityCallbacks (and probably for the better since it was always a rather foreign concept). There is this 'checkObjectAccess' hook, but I've never known what that is and I'd like it if we could remove that also...
Alternatively, I could just make JSBrokenFrameIterator return *all* frames in all contexts and let JSD do the subsumes check. That's trivial on the JS engine, so naturally I like that option.
Flags: needinfo?(luke)
Comment 40•11 years ago
|
||
I think we should add the subsumes hook back. We're going to need it for stack trace filtering anyway.
The hook can just call GetCxSubjectPrincipal(cx)->Subsumes(GetObjectPrincipal(obj)).
Assignee | ||
Comment 41•11 years ago
|
||
Alright, here's a patch that adds the hook to JSSecurityCallbacks and calls it from inside the stack iterator. bholley: could you write the patch that provides JSSubsumesOp, where appropriate, in the browser?
Attachment #820061 -
Attachment is obsolete: true
Attachment #8346706 -
Flags: review?(jdemooij)
Comment 42•11 years ago
|
||
Comment on attachment 8346706 [details] [diff] [review]
check-subsumes
Review of attachment 8346706 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good.
Attachment #8346706 -
Flags: review?(jdemooij) → review+
Comment 43•11 years ago
|
||
Adding in the caps bits. Flagging jandem for review on those.
Attachment #8346706 -
Attachment is obsolete: true
Attachment #8347382 -
Flags: review?(mrbkap)
Updated•11 years ago
|
Attachment #8347382 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 44•11 years ago
|
||
I tested the patch... and of course the subsumes check is inverted (if NOT subsumes, then skip the activation). With that fixed, STR in comment 0 show content in the callstack.
https://tbpl.mozilla.org/?tree=Try&rev=ec6a37404faf
Assignee | ||
Comment 45•11 years ago
|
||
Green except for a static analysis rooting failure which should be fixed by an AutoAssertNoGC around the subsumes call:
https://tbpl.mozilla.org/?tree=Try&rev=1f29e933ef7f
Assignee | ||
Comment 46•11 years ago
|
||
With the AutoAssertNoGC and inverted subsumes check. Jan: this fixes comment 0, but do you suppose you could do a quick check to make sure, in other cases, the callstacks look reasonable?
Attachment #8348891 -
Flags: feedback?(odvarko)
Assignee | ||
Updated•11 years ago
|
Attachment #8347382 -
Attachment is obsolete: true
Assignee | ||
Comment 47•11 years ago
|
||
Green!
Reporter | ||
Comment 48•11 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #46)
> Created attachment 8348891 [details] [diff] [review]
> Bug-924905---Do-a-subsumes-check-inside-the-stack-.patch
>
> With the AutoAssertNoGC and inverted subsumes check. Jan: this fixes
> comment 0, but do you suppose you could do a quick check to make sure, in
> other cases, the callstacks look reasonable?
Sure, any chance to get a try build (Win32, 32 bit)?
Honza
Assignee | ||
Comment 49•11 years ago
|
||
You bet, here's the one from the above try push: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/lwagner@mozilla.com-ec6a37404faf/try-win32/firefox-29.0a1.en-US.win32.zip
Reporter | ||
Comment 50•11 years ago
|
||
Yep, I can see the content frames again!
One question though, I don't know if it's related, but when testing Firebug (with the try build) I needed to comment out following line (Firebug is executing it when halting e.g. at a breakpoint):
stoppedFrame.executionContext.scriptsEnabled = false;
(stoppedFrame is the the JSD frame Firebug stopped in)
It looks like the executionContext is not the content but chrome (or both), since I am experiencing Firebug UI freeze. Has anything related changed? What the scriptsEnabled really does now?
It was used to disable page scripts while the debugger is sitting at a breakpoint.
Honza
Reporter | ||
Comment 51•11 years ago
|
||
More observation.
When using Components.stack within onMutateAttr (attribute mutation event handler), I still don't see the content frames (only chrome frames).
So, Components.stack still seems to be broken.
See it in the example extension attached in comment #7
https://github.com/firebug/manual-tests/blob/master/mozilla/break-on-mutation/content/debugger.js#L101
sysout("onMutateAttr; " + getComponentsStack());
I see content frames only within onDebugger (JSD callback)
https://github.com/firebug/manual-tests/blob/master/mozilla/break-on-mutation/content/debugger.js#L82
Honza
Reporter | ||
Comment 52•11 years ago
|
||
I am also attaching updated test-extension. It now also tries to use JSD2 to get the stack frames. This will also be important for built in Firefox devtools.
The try build: only chrome frames
The current Release 26, also content frames are visible
Follow the same STRs and wath the system console logs (dump). Take a look at "onMutateAttr JSD2 stack" log.
Source code of the extension is here:
https://github.com/firebug/manual-tests/tree/master/mozilla/break-on-mutation
Honza
Reporter | ||
Updated•11 years ago
|
Attachment #8348891 -
Flags: feedback?(odvarko)
Assignee | ||
Comment 53•11 years ago
|
||
(In reply to Jan Honza Odvarko from comment #50)
> One question though, I don't know if it's related, but when testing Firebug
> (with the try build) I needed to comment out following line (Firebug is
> executing it when halting e.g. at a breakpoint):
>
> stoppedFrame.executionContext.scriptsEnabled = false;
I'm not sure where 'stoppedFrame' came from but the patch does change stack iteration such that chrome will see content and chrome frames if iteration starts in chrome. Before bug 862627, which changed all this, perhaps iteration was starting in content and seeing only content and now iteration is starting in chrome and seeing chrome and content?
Components.stack uses a different stack walking mechanism (JS::DescribeStack) than JSD, which explains the breakage. I was a little scared to change JS::DescribeStack since it is pretty widely used, but doing so seems to fix Component.stack. Here is a try push:
https://tbpl.mozilla.org/?tree=Try&rev=9a95afc49eae
Comment 54•11 years ago
|
||
> stoppedFrame.executionContext.scriptsEnabled = false;
Is stoppedFrame the chrome event listener? Because if so, then stoppedFrame.executionContext will be a chrome context.
And the whole executionContext thing will become even more nonsensical as we remove per-window JSContexts altogether...
> It was used to disable page scripts while the debugger is sitting at a breakpoint.
Firebug should disable scripts on the page(s) it cares about directly, I would think. Docshell has a way to do that, iirc: the .allowJavascript property.
DescribeStack is used for content-exposed exceptions, no? I'm not sure we want to mess with it, esp. on a branch.
Assignee | ||
Comment 55•11 years ago
|
||
(In reply to Vacation Dec 19 to Jan 1 from comment #54)
> DescribeStack is used for content-exposed exceptions, no? I'm not sure we
> want to mess with it, esp. on a branch.
Agreed. Looks like some browser-chrome tests fail in the try push for callstack-related reasons.
Does Firebug rely on Components.stack or would it be ok to leave it somewhat broken?
Here's the Win32 build from the above try push: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/lwagner@mozilla.com-9a95afc49eae/try-win32/firefox-29.0a1.en-US.win32.zip
Comment 56•11 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #55)
> Does Firebug rely on Components.stack or would it be ok to leave it somewhat broken?
As far as I can tell, we use Components.stack for:
- showing stack for console.* APIs
- getting the originator of XHRs (within http-on-modify-request and http-on-opening-request observers)
- detecting whether a piece of code was created through eval or some other method, within our JSD debugger service, by checking if there is a caller
I don't think any of these rely on crossing event loop boundaries. (At least, they work on Nightly.)
Assignee | ||
Comment 57•11 years ago
|
||
That's good. So I'm happy to land the patch Jan reviewed (that fixes JS::DescribeStack, but doesn't change Componenents.stack or JSD2), I'm just worried about the problem in comment 50 concerning stoppedFrame.
Comment 58•11 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #57)
> That's good. So I'm happy to land the patch Jan reviewed (that fixes
> JS::DescribeStack, but doesn't change Componenents.stack or JSD2), I'm just
> worried about the problem in comment 50 concerning stoppedFrame.
That's bug 953344, I think, which is basically a separate issue.
Reporter | ||
Comment 59•11 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #53)
> I'm not sure where 'stoppedFrame' came from
'stoppedFrame' the frame Firebug stopped in. The value is coming from JSD backend and passed into JSD hooks, e.g. jsd.breakpointHook.
I'll continue commenting on the problem (comment #50) in bug 953344.
Honza
Assignee | ||
Comment 60•11 years ago
|
||
Ok, great. Just to be sure: does this patch makes the problem in bug 953344 more likely?
Reporter | ||
Comment 61•11 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #60)
> Ok, great. Just to be sure: does this patch makes the problem in bug 953344
> more likely?
My feeling is that bug 953344 is different unrelated issue.
Honza
Comment 62•11 years ago
|
||
> does this patch makes the problem in bug 953344 more likely?
No, it does not.
Assignee | ||
Comment 63•11 years ago
|
||
Assignee | ||
Comment 64•11 years ago
|
||
I should correct comment 57 and say that this landed patch fixes JSBrokenFrameIterator (and thus JSD) but leaves JS::DescribeStack/Components.stack/JSD2 unchanged.
Comment 65•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 66•11 years ago
|
||
HI Luke, IS this ready to be uplifted on branches(aurora/beta) ?
Flags: needinfo?(luke)
Assignee | ||
Comment 67•11 years ago
|
||
Comment on attachment 8348891 [details] [diff] [review]
Bug-924905---Do-a-subsumes-check-inside-the-stack-.patch
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 862627
User impact if declined: bad Firebug behavior
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): medium-to-low, although the risk should be confined to Firebug
Attachment #8348891 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(luke)
Assignee | ||
Comment 68•11 years ago
|
||
The beta patch has some context-only rebase conflicts. Also, there was a build failure since apparently AutoAssertNoGC is not yet on beta. AutoAssertNoGC was only added to silence a static rooting analysis warning which isn't run on beta, so I removed it. Carrying forward r+ since these changes are pretty minor.
[Approval Request Comment]
Same as above.
Attachment #8356403 -
Flags: review+
Attachment #8356403 -
Flags: approval-mozilla-beta?
Updated•11 years ago
|
Attachment #8348891 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
Attachment #8356403 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•11 years ago
|
Comment 69•11 years ago
|
||
Comment 70•11 years ago
|
||
Jan, can you please verify this is working for you now in Firefox Nightly, Aurora, and Beta?
Flags: needinfo?(odvarko)
Reporter | ||
Comment 71•11 years ago
|
||
It already works in Nightly, but the commits (comment #69) didn't make it into the Aurora and Beta channels yet. I'll recheck tomorrow.
Honza
Reporter | ||
Comment 72•11 years ago
|
||
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #70)
> Jan, can you please verify this is working for you now in Firefox Nightly,
> Aurora, and Beta?
Tested and it works on all channels, thanks!
Honza
Flags: needinfo?(odvarko)
You need to log in
before you can comment on or make changes to this bug.
Description
•