Rewrite callers of document.createElement in browser.xul to explicitly create XUL elements

RESOLVED FIXED in Firefox 63

Status

()

P1
normal
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: bgrins, Assigned: mossop)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

8 months ago
One issue that comes up when running the main browser window as xhtml instead of xul is that `document.createElement` does a different thing (as the default ns is html instead of xul).

I've thought about two options for migrating existing code:

a) `createElement(foo)` -> `createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", foo)`
b) `createElement(foo)` -> `createXULElement(foo)` (where createXULElement is a new chrome-only method on Document that's sugar for createElementNS).

Originally I thought (a) would be best in order to stick with web standard APIs, but we'd have to figure out some details around rewriting code to do so. For example: should XUL_NS be defined as a const at the top of each file, or in the individual scope, or a hardcoded string? Alternatively, could we introduce XUL_NS as a global in all chrome contexts (window scope and JSM)?

Going with (b) would be an easier find/replace job that would result in less churn (calls being wrapped to satisfy eslint line length requirement, for instance). It would also give an easy function call to grep for in the future when we want to migrate existing XUL elements to HTML elements. And we could rewrite a bunch of existing frontend code that already does createElementNS with XUL NS to use the more compact call: https://searchfox.org/mozilla-central/search?q=createElementNS(XUL&case=false&regexp=false&path=.
(Reporter)

Comment 1

8 months ago
Regardless of the approach we end up taking, we'll likely want to a build that instruments `createElement` to crash whenever it's called from browser.xul to confirm we haven't missed any callers.
(Reporter)

Comment 2

8 months ago
Any opinions on option (a) vs (b) in Comment 0? Or have another idea for how to handle this that I haven't thought of?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bzbarsky)
(b) is faster than (a) (no need to deal with the namespace string, validate it, etc).

(a) is more error-prone, though if we make XUL_NS a global constant instead of using in-place strings everywhere, that can be mitigated.  Certainly exposing such a constant on chrome windows and the JSM global is easy.

There's maybe an option (c): Flag the document involved as creating XUL elements for createElement.  It's even further from web standard APIs and kinda confusing.

My personal take is that we should do (b), but I _really_ hate the createElementNS API.   ;)
Flags: needinfo?(bzbarsky)

Comment 4

8 months ago
I'd go for (b). The NS variant is awful for readability, very few people know about it if they haven't worked with the plain craziness that is XHTML namespaces, and it leads to the pollution of a dozen-and-one source files with XUL_NS and HTML_NS constants. I wouldn't be surprised if we have at least 2 files defining one of these for browser.js (potentially inside functions, not necessarily globall). The sooner we can stop writing it the better.

Like bz, I really don't like the NS variant, having recently been burned by the arbitrariness of XML namespaces in other people's code in bug 1394941 (see comments there, and uuuuugly hackaround in https://github.com/mozilla/readability/pull/458/files ). (TL;DR: "https://www.w3.org/1999/xhtml" is not "http://www.w3.org/1999/xhtml")
Flags: needinfo?(gijskruitbosch+bugs)

Comment 5

8 months ago
An additional point here is that if we ever need hacks for custom elements or something like that, specific to XUL, it's much easier and performant to stick that in our magical new custom method than in the overloaded createElementNS which we'd "share" with public web-exposed things, with the risk of regressions etc.
(Reporter)

Comment 6

8 months ago
OK - let's go with (b). I'd be happy to stop using createElementNS in the frontend.
Comment hidden (obsolete)
(Reporter)

Updated

8 months ago
See Also: → bug 1479125
Attachment #8995663 - Attachment description: Bug 1479050: Switch from createElement to createXULElement. → Bug 1479050: Add document.createXULElement for creating XUL elements from any document type.
Comment on attachment 8995727 [details]
Bug 1479050: Migrate a number of call-sites to use document.createXULElement.

Brian Grinstead [:bgrins] has approved the revision.

https://phabricator.services.mozilla.com/D2489
Attachment #8995727 - Flags: review+
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Priority: -- → P1
(Reporter)

Comment 12

8 months ago
I remembered there are some callers in browser/modules/webrtcUI.jsm (see https://reviewboard.mozilla.org/r/256118/diff/3#index_header), which aren't tehcnically used in browser.xul but is used by the hidden window on mac (which shares most of the same JS). Could you please include those as well?
(Reporter)

Updated

8 months ago
Blocks: 1479538
Comment on attachment 8995663 [details]
Bug 1479050: Add document.createXULElement for creating XUL elements from any document type.

Boris Zbarsky [:bz] (no decent commit message means r-) has approved the revision.

https://phabricator.services.mozilla.com/D2482
Attachment #8995663 - Flags: review+
(Assignee)

Comment 14

8 months ago
(In reply to Brian Grinstead [:bgrins] from comment #12)
> I remembered there are some callers in browser/modules/webrtcUI.jsm (see
> https://reviewboard.mozilla.org/r/256118/diff/3#index_header), which aren't
> tehcnically used in browser.xul but is used by the hidden window on mac
> (which shares most of the same JS). Could you please include those as well?

I'm going to get the C++ changes landed along with the existing migrations you've reviewed then follow-up with further migration patches since those will be able to be tested with artifact builds which will make my life easier.
(Reporter)

Comment 15

8 months ago
(In reply to Dave Townsend [:mossop] from comment #14)
> I'm going to get the C++ changes landed along with the existing migrations
> you've reviewed then follow-up with further migration patches since those
> will be able to be tested with artifact builds which will make my life
> easier.

Sounds good to me

Comment 16

8 months ago
Pushed by dtownsend@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d4b0cb363a4
Add document.createXULElement for creating XUL elements from any document type. r=bz

Comment 17

8 months ago
Pushed by dtownsend@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/701d67d48f5d
Migrate a number of call-sites to use document.createXULElement. r=bgrins
Backed out for xpcshell failures on test_VariablesView_filtering-without-controller.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-searchStr=xpc&fromchange=701d67d48f5d79a1f6e93416949fa6d18f7f92fe&selectedJob=191021176

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=191021176&repo=mozilla-inbound&lineNumber=2085

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/cd5678331e973d2ea13191578f3d974f354824b7

[task 2018-07-31T01:04:09.155Z] 01:04:09     INFO -  TEST-START | devtools/client/shared/test/unit/test_VariablesView_filtering-without-controller.js
[task 2018-07-31T01:04:09.377Z] 01:04:09  WARNING -  TEST-UNEXPECTED-FAIL | devtools/client/shared/test/unit/test_VariablesView_filtering-without-controller.js | xpcshell return code: -11
[task 2018-07-31T01:04:09.378Z] 01:04:09     INFO -  TEST-INFO took 226ms
[task 2018-07-31T01:04:09.379Z] 01:04:09     INFO -  >>>>>>>
[task 2018-07-31T01:04:09.379Z] 01:04:09     INFO -  PID 9532 | JavaScript strict warning: resource://devtools/shared/Loader.jsm, line 224: ReferenceError: reference to undefined property "name"
[task 2018-07-31T01:04:09.380Z] 01:04:09     INFO -  PID 9532 | JavaScript strict warning: resource://devtools/shared/base-loader.js -> resource://devtools/shared/node-properties/node-properties.js, line 134: ReferenceError: reference to undefined property "resume"
[task 2018-07-31T01:04:09.381Z] 01:04:09     INFO -  PID 9532 | JavaScript strict warning: resource://devtools/shared/base-loader.js -> resource://devtools/shared/node-properties/node-properties.js, line 124: ReferenceError: reference to undefined property "f"
[task 2018-07-31T01:04:09.381Z] 01:04:09     INFO -  PID 9532 | JavaScript strict warning: resource://devtools/shared/base-loader.js -> resource://devtools/shared/node-properties/node-properties.js, line 130: ReferenceError: reference to undefined property "f"
[task 2018-07-31T01:04:09.383Z] 01:04:09     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2018-07-31T01:04:09.383Z] 01:04:09     INFO -  TEST-PASS | devtools/client/shared/test/unit/test_VariablesView_filtering-without-controller.js | run_test - [run_test : 21] Got a container. - {} == true
[task 2018-07-31T01:04:09.383Z] 01:04:09     INFO -  PID 9532 | ExceptionHandler::GenerateDump cloned child 9546
[task 2018-07-31T01:04:09.384Z] 01:04:09     INFO -  PID 9532 | ExceptionHandler::SendContinueSignalToChild sent continue signal to child
[task 2018-07-31T01:04:09.385Z] 01:04:09     INFO -  PID 9532 | ExceptionHandler::WaitForContinueSignal waiting for continue signal...
[task 2018-07-31T01:04:09.385Z] 01:04:09     INFO -  <<<<<<<
[task 2018-07-31T01:04:09.385Z] 01:04:09     INFO -  mozcrash Downloading symbols from: https://queue.taskcluster.net/v1/task/MF5_TpBMRmqm93MHPiyQsA/artifacts/public/build/target.crashreporter-symbols.zip
[task 2018-07-31T01:04:15.624Z] 01:04:15     INFO -  mozcrash Copy/paste: /usr/local/bin/linux64-minidump_stackwalk /tmp/xpc-other-0le1Dt/7bb5561a-b79e-6479-beb7-55264e40ad6c.dmp /tmp/tmp4DwbGS
[task 2018-07-31T01:04:24.410Z] 01:04:24     INFO -  mozcrash Saved minidump as /builds/worker/workspace/build/blobber_upload_dir/7bb5561a-b79e-6479-beb7-55264e40ad6c.dmp
[task 2018-07-31T01:04:24.410Z] 01:04:24     INFO -  mozcrash Saved app info as /builds/worker/workspace/build/blobber_upload_dir/7bb5561a-b79e-6479-beb7-55264e40ad6c.extra
[task 2018-07-31T01:04:24.411Z] 01:04:24  WARNING -  PROCESS-CRASH | devtools/client/shared/test/unit/test_VariablesView_filtering-without-controller.js | application crashed [@ nsWrapperCache::GetWrapper() const]
[task 2018-07-31T01:04:24.412Z] 01:04:24     INFO -  Crash dump filename: /tmp/xpc-other-0le1Dt/7bb5561a-b79e-6479-beb7-55264e40ad6c.dmp
[task 2018-07-31T01:04:24.412Z] 01:04:24     INFO -  Operating system: Linux
[task 2018-07-31T01:04:24.413Z] 01:04:24     INFO -                    0.0.0 Linux 4.4.0-1014-aws #14taskcluster1-Ubuntu SMP Tue Apr 3 10:27:00 UTC 2018 x86_64
[task 2018-07-31T01:04:24.413Z] 01:04:24     INFO -  CPU: x86
[task 2018-07-31T01:04:24.413Z] 01:04:24     INFO -       GenuineIntel family 6 model 62 stepping 4
[task 2018-07-31T01:04:24.414Z] 01:04:24     INFO -       2 CPUs
[task 2018-07-31T01:04:24.414Z] 01:04:24     INFO -  GPU: UNKNOWN
[task 2018-07-31T01:04:24.415Z] 01:04:24     INFO -  Crash reason:  SIGSEGV
[task 2018-07-31T01:04:24.418Z] 01:04:24     INFO -  Crash address: 0x8
[task 2018-07-31T01:04:24.418Z] 01:04:24     INFO -  Process uptime: not available
[task 2018-07-31T01:04:24.419Z] 01:04:24     INFO -  Thread 0 (crashed)
[task 2018-07-31T01:04:24.419Z] 01:04:24     INFO -   0  libxul.so!nsWrapperCache::GetWrapper() const [nsWrapperCacheInlines.h:701d67d48f5d79a1f6e93416949fa6d18f7f92fe : 30 + 0x1d]
[task 2018-07-31T01:04:24.420Z] 01:04:24     INFO -      eip = 0xf1c5877d   esp = 0xffd303d0   ebp = 0xffd30418   ebx = 0xf75f6000
[task 2018-07-31T01:04:24.420Z] 01:04:24     INFO -      esi = 0xf75f6000   edi = 0x00000004   eax = 0x00000004   ecx = 0xf741b174
[task 2018-07-31T01:04:24.422Z] 01:04:24     INFO -      edx = 0xeac10800   efl = 0x00210296
[task 2018-07-31T01:04:24.426Z] 01:04:24     INFO -      Found by: given as instruction pointer in context
Flags: needinfo?(dtownsend)

Comment 19

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8d4b0cb363a4
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
(Reporter)

Updated

8 months ago
Depends on: 1479866

Comment 20

8 months ago
Pushed by dtownsend@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/88f6fc95ef0e
Migrate a number of call-sites to use document.createXULElement. r=bgrins
(Assignee)

Updated

8 months ago
Status: RESOLVED → REOPENED
Flags: needinfo?(dtownsend)
Resolution: FIXED → ---

Comment 21

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/88f6fc95ef0e
Status: REOPENED → RESOLVED
Last Resolved: 8 months ago8 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.