Open
Bug 1103327
Opened 10 years ago
Updated 2 years ago
AutoJSAPI should block out previous script setting stack entries
Categories
(Core :: XPConnect, defect)
Tracking
()
REOPENED
People
(Reporter: bholley, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
6.18 KB,
patch
|
bobowen
:
review+
|
Details | Diff | Splinter Review |
Right now AutoJSAPI doesn't interact with the script settings stack at all, making it slightly incongruous with AutoNoJSAPI, and leading to odd situations where GetEntryGlobal() will reach through any number of AutoJSAPIs to find an AutoEntryScript. I think we should fix this. Let's see if anything breaks.
Reporter | ||
Comment 1•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=03e70923c912
It looks like your try push might be hitting bug 1104160.
Reporter | ||
Comment 3•10 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #2) > It looks like your try push might be hitting bug 1104160. Sure looks like it.
Depends on: 1104160
Reporter | ||
Comment 4•10 years ago
|
||
Attachment #8527823 -
Flags: review?(bugs)
Attachment #8527823 -
Flags: review?(bobowencode)
Comment 5•10 years ago
|
||
Do we want this behavior? If there is script settings entry on stack and an event is dispatch, the stuff under event listener won't see that script settings entry?
Comment 6•10 years ago
|
||
What does the spec say about this case?
Reporter | ||
Comment 7•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #5) > Do we want this behavior? I think so, but I'm interested in your opinion. > If there is script settings entry on stack and an event is dispatch, > the stuff under event listener won't see that script settings entry? It will, because event dispatch uses AutoEntryScript. This only affects uses of the raw AutoJSAPI, which is never supposed to trigger script. (In reply to Olli Pettay [:smaug] from comment #6) > What does the spec say about this case? Nothing, because the spec doesn't have a concept of AutoJSAPI. AutoJSAPI basically means "give me a clean fresh JSAPI state, ignoring what's on the stack above me". In cases where callers do that, it seems much safer IMO to avoid inheriting the entry point.
Comment 8•10 years ago
|
||
Comment on attachment 8527823 [details] [diff] [review] Make AutoJSAPI do the same thing to the script settings stack as AutoNoJSAPI. v1 Review of attachment 8527823 [details] [diff] [review]: ----------------------------------------------------------------- Just the NoJSAPI bit perhaps needs fixing. I wasn't quite sure about this, but after your answers to smaug this makes sense to me. I suppose it goes some way to protecting things until we can ban running script without an AutoEntryScript. ::: dom/base/ScriptSettings.cpp @@ +295,5 @@ > return cx; > } > > AutoJSAPI::AutoJSAPI() > + : ScriptSettingsStackEntry() I suppose there is no problem that we push on construction and, for example if an Init() fails, we don't pop until we go out of scope? ::: dom/base/ScriptSettings.h @@ +157,5 @@ > > public: > ~ScriptSettingsStackEntry(); > > bool NoJSAPI() { return !mGlobalObject; } Should NoJSAPI be renamed with these changes, perhaps to NoEntryPoint or something? The comment would need updating on cpp line 39 as well.
Attachment #8527823 -
Flags: review?(bobowencode) → review+
Comment 9•10 years ago
|
||
Comment on attachment 8527823 [details] [diff] [review] Make AutoJSAPI do the same thing to the script settings stack as AutoNoJSAPI. v1 Based on the IRC discussion, it is not at all clear to me we want this before we've fixed dictionary.init and such.
Attachment #8527823 -
Flags: review?(bugs)
Reporter | ||
Comment 10•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #9) > Comment on attachment 8527823 [details] [diff] [review] > Make AutoJSAPI do the same thing to the script settings stack as > AutoNoJSAPI. v1 > > Based on the IRC discussion, it is not at all clear to me we want this before > we've fixed dictionary.init and such. This change doesn't prevent us from running scripted getters etc in those cases - it just prevents the Entry Settings Object from leaking through. That seems to me like a clear win - is there any reason why it wouldn't be?
Flags: needinfo?(bugs)
Comment 11•10 years ago
|
||
The getter code wouldn't see the latest entry script. And if I read the code correctly, http://mxr.mozilla.org/mozilla-central/source/dom/alarm/AlarmsManager.js#65 kind of stuff would start to throw an exception if run inside a getter.
Flags: needinfo?(bugs)
Reporter | ||
Updated•8 years ago
|
Assignee: bobbyholley → nobody
Comment 12•6 years ago
|
||
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INACTIVE
Updated•6 years ago
|
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•