Brian Grinstead [:bgrins] Reporter
	Description • 6 years 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=.

	Brian Grinstead [:bgrins] Reporter
	Comment 1 • 6 years 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.

	Brian Grinstead [:bgrins] Reporter
	Comment 2 • 6 years 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?

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 6 years ago

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

	:Gijs (he/him)
	Comment 4 • 6 years 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")

	:Gijs (he/him)
	Comment 5 • 6 years 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.

	Brian Grinstead [:bgrins] Reporter
	Comment 6 • 6 years ago

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

	Dave Townsend [:mossop] Assignee
	Comment 10 • 6 years ago

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

	Phabricator Automation
	Comment 11 • 6 years ago

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

	Brian Grinstead [:bgrins] Reporter
	Comment 12 • 6 years 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?

	Phabricator Automation
	Comment 13 • 6 years ago

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

	Dave Townsend [:mossop] Assignee
	Comment 14 • 6 years 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.

	Brian Grinstead [:bgrins] Reporter
	Comment 15 • 6 years 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

	Pulsebot
	Comment 16 • 6 years 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

	Pulsebot
	Comment 17 • 6 years 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

	Cosmin Sabou [:CosminS]
	Comment 18 • 6 years ago

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

	Stefan Hindli [:stefan_hindli]
	Comment 19 • 6 years ago
bugherder

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

	Pulsebot
	Comment 20 • 6 years 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

	Dorel Luca [:dluca]
	Comment 21 • 6 years ago
bugherder

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