Open Bug 1103327 Opened 10 years ago Updated 2 years ago

AutoJSAPI should block out previous script setting stack entries

Categories

(Core :: XPConnect, defect)

x86
macOS
defect

Tracking

()

REOPENED

People

(Reporter: bholley, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
It looks like your try push might be hitting bug 1104160.
(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
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?
What does the spec say about this case?
(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 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 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)
(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)
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)
Blocks: 1207693
Assignee: bobbyholley → nobody
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
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: