Closed
Bug 796938
Opened 12 years ago
Closed 10 years ago
Remove usage of GetDynamicScriptContext
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: bzbarsky, Assigned: bholley)
References
(Blocks 2 open bugs)
Details
Attachments
(13 files, 1 obsolete file)
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
We should understand this, so we can sort out what needs to go in the script entry point stack.
Assignee | ||
Comment 2•12 years ago
|
||
nsContentUtils.cpp: -Used for determining whether a given nsJSContext is on the stack, which sets mScriptEvaluting. nsGlobalWindow.cpp: -Used to get the dynamic script global -Used to determine whether |this|'s cx is on the top of the stack. -Used to get the dynamic script global nsJSEnvironment.cpp -Used to get the dynamic script global nsLocation.cpp -Used to get the topmost cx on the stack that has a dynamic script global -Something tricky with GetProcessingScriptTag nsJSUtils.cpp -Used to implement GetDynamicScriptGlobal nsWindowWatcher.cpp -Used to get a kungFuDeathGrip on a JSContext stored in an RAII struct. -Used to get the dynamic script global
Assignee | ||
Comment 3•12 years ago
|
||
So at first glance, this looks pretty encouraging. Pretty much everybody just wants to know the dynamic script global, which is probably what we're going to end up storing on the script entry point stack. So I think the first step here is to remove all uses of GetDynamicScriptContext and replace them with calls to GetDynamicScriptGlobal. Then, we can re-implement GetDynamicScriptGlobal with the script entry point stack, and kill context pushing altogether. Morphing this bug appropriately.
Depends on: 767938
Summary: Figure out what GetDynamicScriptContext and GetDynamicScriptGlobal callers are really trying to do → Remove usage of GetDynamicScriptContext
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 4•12 years ago
|
||
So, it looks like the wackiness in nsLocation goes away with the patch in bug 754029. So we just need to get that landed.
Depends on: 754029
Assignee | ||
Comment 5•12 years ago
|
||
Ah crap. Looks like there's _also_ GetScriptContextFromJSContext, which is a whole other can of worms.
Assignee | ||
Comment 6•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=e6958eb02b93
Assignee | ||
Comment 7•12 years ago
|
||
This was my original patch to remove the API. Since bug 754029 was backed out, this no longer compiles for me with my local patch stack, so I'm posting it here before reverting it.
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 9•10 years ago
|
||
Actually, I think this is a good chunk size to exist on its own.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Assignee | ||
Comment 10•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=21673dc75fd4
Assignee | ||
Comment 11•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=5dbca6c51540
Updated•10 years ago
|
Status: REOPENED → NEW
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 12•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=63792f83d156
Assignee | ||
Comment 13•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ded6b3a40a0b
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8474291 -
Flags: review?(bugs)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8474292 -
Flags: review?(bugs)
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8474293 -
Flags: review?(bugs)
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8474294 -
Flags: review?(bugs)
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8474295 -
Flags: review?(bugs)
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8474296 -
Flags: review?(bugs)
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8474297 -
Flags: review?(bugs)
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8474298 -
Flags: review?(bugs)
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8474299 -
Flags: review?(bugs)
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8474300 -
Flags: review?(bugs)
Assignee | ||
Updated•10 years ago
|
Attachment #713838 -
Attachment is obsolete: true
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8474302 -
Flags: review?(bugs)
Assignee | ||
Comment 25•10 years ago
|
||
We leave this flag on the script context for now - we can move it somewhere else later.
Attachment #8474303 -
Flags: review?(bugs)
Assignee | ||
Comment 26•10 years ago
|
||
Attachment #8474304 -
Flags: review?(bugs)
Comment 27•10 years ago
|
||
Comment on attachment 8474291 [details] [diff] [review] Part 1 - Switch nsGlobalWindow::Focus to GetEntryGlobal and eliminate nsContentUtils::GetWindowFromCaller. v1 (I wonder where the word 'Dynamic' comes to GetDynamicScriptGlobal. Nothing dynamic there, based on the implementation.)
Attachment #8474291 -
Flags: review?(bugs) → review+
Comment 28•10 years ago
|
||
Comment on attachment 8474292 [details] [diff] [review] Part 2 - Switch nsHTMLDocument::Open and XMLDocument::Load to a new GetEntryDocument API and remove nsContentUtils::GetDocumentFromContext. v1 Hmm, I wish Get*Global and GetEntryDocument() weren't global methods. Making them static methods of some class would make the code easier to read. File a followup for that.
Attachment #8474292 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8474293 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8474294 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8474295 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8474296 -
Flags: review?(bugs) → review+
Comment 29•10 years ago
|
||
Comment on attachment 8474297 [details] [diff] [review] Part 7 - Switch nsLocation::GetSourceBaseURL to GetEntryDocument. v1 >- if (!sgo && GetDocShell()) { >- sgo = GetDocShell()->GetScriptGlobalObject(); >+ if (!doc) { >+ nsCOMPtr<nsPIDOMWindow> docShellWin; >+ if (GetDocShell()) { >+ docShellWin = do_QueryInterface(GetDocShell()->GetScriptGlobalObject()); >+ } >+ if (docShellWin) { >+ doc = docShellWin->GetDoc(); >+ } A bit oddly written. Why not if (!doc && GetDocShell()) { nsCOMPtr<nsPIDOMWindow> docShellWin = do_QueryInterface(GetDocShell()->GetScriptGlobalObject()); if (docShellWin) { doc = docShellWin->GetDoc(); } }
Attachment #8474297 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8474298 -
Flags: review?(bugs) → review+
Comment 30•10 years ago
|
||
Comment on attachment 8474299 [details] [diff] [review] Part 9 - Use GetEntryGlobal in nsGlobalWindow::FireAbuseEvents. v1 We should use incumbent global here, I think, but since the old code is using dynamic script context. Perhaps file a followup?
Attachment #8474299 -
Flags: review?(bugs) → review+
Comment 31•10 years ago
|
||
Comment on attachment 8474300 [details] [diff] [review] Part 10 - Use GetEntryGlobal in NS_ScriptErrorReporter. v1 Why can we replace getting global from cx here with GetEntryGlobal()? Doesn't look right to me, at least from API perspective. We're given a cx, and should use that to get the global.
Attachment #8474300 -
Flags: review?(bugs) → review-
Updated•10 years ago
|
Attachment #8474302 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8474303 -
Flags: review?(bugs) → review+
Comment 32•10 years ago
|
||
Comment on attachment 8474304 [details] [diff] [review] Part 13 - Remove GetDynamicScriptContext API. v1 !
Attachment #8474304 -
Flags: review?(bugs) → review+
Reporter | ||
Comment 33•10 years ago
|
||
> I wonder where the word 'Dynamic' comes to GetDynamicScriptGlobal
Dynamic as in "whatever global we happened to enter script on" as opposed to "static" which is "the global of the currently running script". Think dynamic vs static scoping.
Assignee | ||
Comment 34•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #31) > Comment on attachment 8474300 [details] [diff] [review] > Part 10 - Use GetEntryGlobal in NS_ScriptErrorReporter. v1 > > Why can we replace getting global from cx here with GetEntryGlobal()? > Doesn't look right to me, at least from API perspective. Can you explain what you mean here? Are you suggesting that we use GetCurrentGlobal() instead of GetEntryGlobal()? I think that would break things. > We're given a cx, and should use that to get the global. From a practical perspective, the cx we have here is going to be stack-top, and therefore the same one used by the ScriptSettings.cpp machinery, so it doesn't matter whether we pass it explicitly or not. From an API perspective, the whole point of these patches is to stop getting global objects from particular JSContexts, so that we can switch to just using one JSContext for Gecko and (eventually) remove JSContexts altogether. Note that in bug 981198, we're going to move the error reporter off the JSContext entirely.
Flags: needinfo?(bugs)
Comment 35•10 years ago
|
||
What guarantees the cx is from stack-top? NS_ScriptErrorReporter eventually probably take some other param than cx. Global JS object perhaps.
Flags: needinfo?(bugs)
Assignee | ||
Comment 36•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #35) > What guarantees the cx is from stack-top? We have extremely strong assertions (both inside and outside the JS engine) that we only ever operate with the stack-top cx. > NS_ScriptErrorReporter eventually probably take some other param than cx. > Global JS object perhaps. My plan is to eliminate it in its current form with bug 981187, and have AutoJSAPI take care of reporting or propagating exceptions. The eventual error reporting function will probably accept a global object, but we can't do that as long as the signature is still JSErrorReporter. Can you be more specific as to what you want to see in part 10?
Comment 37•10 years ago
|
||
Comment on attachment 8474300 [details] [diff] [review] Part 10 - Use GetEntryGlobal in NS_ScriptErrorReporter. v1 Ok, can you at least assert that cx is the stack-top ?
Attachment #8474300 -
Flags: review- → review+
Assignee | ||
Comment 38•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=25fff355894f
Assignee | ||
Comment 39•10 years ago
|
||
Note that there was a green try push in comment 13. The only substantive code change since then is the addition of an assertion in NS_ScriptErrorReporter.
Comment 40•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5d6a67963f48 https://hg.mozilla.org/mozilla-central/rev/768cb6193db5 https://hg.mozilla.org/mozilla-central/rev/8cff1cd635f9 https://hg.mozilla.org/mozilla-central/rev/6fda5f5e9a47 https://hg.mozilla.org/mozilla-central/rev/1317e7b98dbf https://hg.mozilla.org/mozilla-central/rev/4c95deee6bcd https://hg.mozilla.org/mozilla-central/rev/8b1137d02c20 https://hg.mozilla.org/mozilla-central/rev/abc88c4f9bb7 https://hg.mozilla.org/mozilla-central/rev/7e06192172e0 https://hg.mozilla.org/mozilla-central/rev/f1552d58d314 https://hg.mozilla.org/mozilla-central/rev/9b57ef374bfe https://hg.mozilla.org/mozilla-central/rev/129edf96797e https://hg.mozilla.org/mozilla-central/rev/25fff355894f
Status: NEW → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•