Consider moving execCommand, queryCommand* and designMode from HTMLDocument to Document
Categories
(Core :: DOM: Core & HTML, defect, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox69 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: ehsan.akhgari)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, site-compat)
Attachments
(4 files)
|
||
Updated•7 years ago
|
|
||
Updated•7 years ago
|
|
||
Updated•6 years ago
|
|
Assignee | |
Comment 1•6 years ago
|
||
I have a patch set for all of these together since the underlying code is hard to disentangle.
|
|
Assignee | |
Comment 4•6 years ago
|
||
|
|
Assignee | |
Comment 5•6 years ago
|
||
|
|
Assignee | |
Comment 6•6 years ago
|
||
This is similar to this Chromium change:
https://codereview.chromium.org/1155353002
The editing spec doesn't mention editing non-HTML documents, and historically
this is only a feature that WebKit has supported. Supporting this feature
increases the attack surface of the browser without a clear benefit, so it
seems like a good idea to converge on this behaviour.
|
||
Comment 10•6 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/7910719718f4
https://hg.mozilla.org/mozilla-central/rev/060d26dc9d2f
https://hg.mozilla.org/mozilla-central/rev/9b8cf182a743
|
||
Comment 12•6 years ago
|
||
I suspect that on win7, this has triggered loading msctf.dll when running xperf (it shows up in the IO instrumentation that that talos test runs), where we didn't do so before. I came across this in https://bugzilla.mozilla.org/show_bug.cgi?id=1559878#c6 . I'm not yet sure whether this bug or the e10s pgo changes were responsible for the startup IO regression on win7 there (a whole range of builds is busted so it's hard to tell), but it seemed worth asking either way whether the additional dll load is expected and/or whether there's optimization we can do here to avoid that?
|
|
Assignee | |
Comment 13•6 years ago
|
||
(In reply to :Gijs (he/him) from comment #12)
I suspect that on win7, this has triggered loading msctf.dll when running xperf (it shows up in the IO instrumentation that that talos test runs), where we didn't do so before. I came across this in https://bugzilla.mozilla.org/show_bug.cgi?id=1559878#c6 . I'm not yet sure whether this bug or the e10s pgo changes were responsible for the startup IO regression on win7 there (a whole range of builds is busted so it's hard to tell), but it seemed worth asking either way whether the additional dll load is expected and/or whether there's optimization we can do here to avoid that?
Hmm, that is very strange. Definitely not something that I expected. Looking at our code it seems like we use msctf.dll for IME handling, and we try to see if it's loaded using GetModuleHandleEx but I'm not sure where else we call these functions from. Masayuki, perhaps you would know?
In thoery at least, what this bug did should not have caused any new code to be executed where it wouldn't have been before, and if that's not the case then we have a bug on our hands...
|
|
||
Comment 14•6 years ago
|
||
FWIW, I trypushed from recent-ish central in https://bugzilla.mozilla.org/show_bug.cgi?id=1559878#c9 to see if this was the e10s-for-pgo change's fault, and neither log (ie neither central nor central with the use-e10s-for-pgo stuff backed out) has it now... so either the load has disappeared since then (ie something else has fixed that particular oddity) or it's intermittent (in which case it may or may not be correlated to this patch, depending on whether it happened before or not, which we don't know). Given that this tidbit might still be interesting (perhaps irrespective of whether it's actually caused by this bug), I'll leave ni's intact on this bug.
It's called at first nsWindow::Create() call. I think that it's necessary to be done before first widget gets focus since it's used to setting input context for IME and keyboards. For example, the information is used by touch keyboard to consider using normal mode, URL mode and password mode.
|
|
Assignee | |
Comment 16•6 years ago
|
||
Hmm, but we don't load the DLL from there, right? Only try to access it if it's already loaded...
(In reply to :Ehsan Akhgari from comment #16)
Hmm, but we don't load the DLL from there, right? Only try to access it if it's already loaded...
In my understanding, GetModuleHandleExW() loads DLL if specified one hasn't been loaded yet.
And I think that this cost is not so important because IIRC, this path runs only on Windows Server or something which TSF hasn't been installed into. Even on Win7 for en-US, TSF is installed and enabled for "Tablet PC Input Panel" so that the fallback path won't be used in actual users' environment.
|
|
Assignee | |
Comment 18•6 years ago
|
||
(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(I'm back, but still struggling with the pain) from comment #17)
(In reply to :Ehsan Akhgari from comment #16)
Hmm, but we don't load the DLL from there, right? Only try to access it if it's already loaded...
In my understanding,
GetModuleHandleExW()loads DLL if specified one hasn't been loaded yet.
No, see https://docs.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-getmodulehandlew. This function only returns a handle to an already loaded module without incrementing the module's refcount. If the module hans't been loaded yet, it will return NULL.
And I think that this cost is not so important because IIRC, this path runs only on Windows Server or something which TSF hasn't been installed into. Even on Win7 for en-US, TSF is installed and enabled for "Tablet PC Input Panel" so that the fallback path won't be used in actual users' environment.
I see. So I think the actual loading of the DLL probably happens in some other code entirely, most likely in some Windows code that we call into and isn't something we could easily find by looking through searchfox. :-/
|
|
||
Updated•6 years ago
|
Description
•