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

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P1)

Description

[Brian Grinstead [:bgrins]]

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=.

Comment 1

[Brian Grinstead [:bgrins]]

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.

Comment 2

[Brian Grinstead [:bgrins]]

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?

Comment 3

[Boris Zbarsky [: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.   ;)

Comment 4

[:Gijs (out for now; he/him)]

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")

Comment 5

[:Gijs (out for now; he/him)]

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.

Comment 6

[Brian Grinstead [:bgrins]]

OK - let's go with (b). I'd be happy to stop using createElementNS in the frontend.

Comment 7

[Dave Townsend [:mossop]]

Attachment: Bug 1479050: Add document.createXULElement for creating XUL elements from any document type.

Comment 8

[Dave Townsend [:mossop]]

(In reply to Dave Townsend [:mossop] from comment #7)
> Created attachment 8995663 [details]
> Bug 1479050: Switch from createElement to createXULElement.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=632d3b64deb19b9c7d01acfbe1e5565da475d593

Comment 9

[Dave Townsend [:mossop]]

Attachment: Bug 1479050: Migrate a number of call-sites to use document.createXULElement.

Comment 10

[Dave Townsend [:mossop]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b13d3eca81b82aacf232b9627690dcbdfad53fc

Comment 11

[Phabricator Automation]

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

Comment 12

[Brian Grinstead [:bgrins]]

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?

Comment 13

[Phabricator Automation]

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

Comment 14

[Dave Townsend [:mossop]]

(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.

Comment 15

[Brian Grinstead [:bgrins]]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d4b0cb363a4
Add document.createXULElement for creating XUL elements from any document type. r=bz

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/701d67d48f5d
Migrate a number of call-sites to use document.createXULElement. r=bgrins

Comment 18

[Cosmin Sabou [:CosminS]]

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

Comment 19

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/8d4b0cb363a4

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88f6fc95ef0e
Migrate a number of call-sites to use document.createXULElement. r=bgrins

Comment 21

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/88f6fc95ef0e