Brian Grinstead [:bgrins] Assignee
	Description • 6 years ago

One issue that comes up when running the main browser window as xhtml instead of xul is that in XUL we drop whitespace nodes so we assume `childNodes` are elements. This leads to some pretty early and obvious failures where we get ahold of a text node instead of an element.

I believe the behavior can be changed so that we use the "element" variation of these APIs (firstElementChild, lastElementChild, and children).

However, one type of `childNodes` access we probably still want to allow is:

```
while (foo.firstChild)
  foo.firstChild.remove();
```

As switching this to `firstElementChild` would change the behavior between XUL and HTML.

	Brian Grinstead [:bgrins] Assignee
	Comment 5 • 6 years ago

Did a try push to log every call to each of these methods in browser.xul. There are separate chunks but I suspect if we collect all calls from one of the chunks and migrate them that will cover most cases.

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

(In reply to Brian Grinstead [:bgrins] from comment #5)
> Did a try push to log every call to each of these methods in browser.xul.
> There are separate chunks but I suspect if we collect all calls from one of
> the chunks and migrate them that will cover most cases.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31ed5976c1afab91ebd2ffcb266e4996358fd7a3, which leads to logs like https://taskcluster-artifacts.net/TT2AjTORSc6HRnGPeKI2Kg/0/public/logs/live_backing.log

	Brian Grinstead [:bgrins] Assignee
	Comment 10 • 6 years ago

Florian has some scripts to rewrite JS across the tree in a more fine-grained manner at https://bitbucket.org/fqueze/xpcshell-rewrites/src. If we can get a narrow list of files to change the more blunt approach with some manual fixups may still work, but using a script like that would give more control over what changes.

	Brian Grinstead [:bgrins] Assignee
	Comment 14 • 6 years ago

New version built with a whitelist of non-test files, generated by:

- From https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe30af48e906857cbd5ced3c4e3807eb63f30929&selectedJob=191220216, load https://taskcluster-artifacts.net/LJ24k2N1QYypzBkv0qwAIA/0/public/logs/live_backing.log and save it.
- Run `grep -o 'WebIDL.*\[.*\"' live_backing.log | cut -f3- -d: | sort -u`

Still awaiting results from a version with a full instrumentation run (which does all b-c tests in a single chunk) to make sure it isn't missing any.

	Brian Grinstead [:bgrins] Assignee
	Comment 15 • 6 years ago

Splitting the logging patch out from mozreview

	Brian Grinstead [:bgrins] Assignee
	Comment 18 • 6 years ago

Instrumentation job is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3c020c3edc55372deaeddc8a5dcaf4104546f40&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=191413504.

Data gathered from that using process in Comment 14:

    ///modules/AsyncTabSwitcher.jsm
    ///modules/CustomizableUI.jsm
    ///modules/CustomizableWidgets.jsm
    ///modules/CustomizeMode.jsm
    ///modules/DownloadsSubview.jsm
    ///modules/DragPositionManager.jsm
    ///modules/ExtensionsUI.jsm
    ///modules/PanelMultiView.jsm
    ///modules/TabsList.jsm
    ///modules/UITour.jsm
    ///modules/syncedtabs/TabListView.js
    ///modules/webrtcUI.jsm
    //browser/content/browser-addons.js
    //browser/content/browser-ctrlTab.js
    //browser/content/browser-customization.js
    //browser/content/browser-pageActions.js
    //browser/content/browser-sidebar.js
    //browser/content/browser-siteIdentity.js
    //browser/content/browser-sync.js
    //browser/content/browser.js
    //browser/content/customizableui/panelUI.js
    //browser/content/customizableui/toolbar.xml
    //browser/content/downloads/downloads.js
    //browser/content/parent/ext-menus.js
    //browser/content/places/browserPlacesViews.js
    //browser/content/places/controller.js
    //browser/content/places/editBookmark.js
    //browser/content/search/search.xml
    //browser/content/tabbrowser.js
    //browser/content/tabbrowser.xml
    //browser/content/urlbarBindings.xml
    //browser/content/utilityOverlay.js
    //global/content/bindings/autocomplete.xml
    //global/content/bindings/findbar.xml
    //global/content/bindings/menu.xml
    //global/content/bindings/menulist.xml
    //global/content/bindings/popup.xml
    //global/content/bindings/richlistbox.xml
    //global/content/bindings/scrollbox.xml
    //global/content/bindings/tabbox.xml
    //global/content/bindings/textbox.xml
    //global/content/elements/general.js
    //global/content/printPreviewToolbar.js
    //gre/modules/ContextualIdentityService.jsm
    //gre/modules/PageMenu.jsm
    //gre/modules/PopupNotifications.jsm
    //gre/modules/SelectParentHelper.jsm
    //gre/modules/addons/XPIProvider.jsm -> jar:file:///builds/worker/workspace/build/application/firefox/browser/features/firefox@getpocket.com.xpi!/bootstrap.js
    //gre/modules/addons/XPIProvider.jsm -> jar:file:///builds/worker/workspace/build/application/firefox/browser/features/screenshots@mozilla.org.xpi!/bootstrap.js
    //mochikit/content/browser-test.js
    //mochitests/content/browser/browser/base/content/test/general/browser_bug1299667.js
    //mochitests/content/browser/browser/base/content/test/general/browser_bug462673.js
    //mochitests/content/browser/browser/base/content/test/general/browser_bug647886.js
    //mochitests/content/browser/browser/base/content/test/general/browser_contextmenu_childprocess.js
    //mochitests/content/browser/browser/base/content/test/general/browser_ctrlTab.js
    //mochitests/content/browser/browser/base/content/test/general/browser_decoderDoctor.js
    //mochitests/content/browser/browser/base/content/test/general/browser_page_style_menu.js
    //mochitests/content/browser/browser/base/content/test/performance/browser_tabstrip_overflow_underflow.js
    //mochitests/content/browser/browser/base/content/test/performance/browser_urlbar_search.js
    //mochitests/content/browser/browser/base/content/test/permissions/browser_canvas_fingerprinting_resistance.js
    //mochitests/content/browser/browser/base/content/test/permissions/browser_permissions.js
    //mochitests/content/browser/browser/base/content/test/permissions/browser_temporary_permissions.js
    //mochitests/content/browser/browser/base/content/test/permissions/browser_temporary_permissions_expiry.js
    //mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_crashreporting.js
    //mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js
    //mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_notificationBar.js
    //mochitests/content/browser/browser/base/content/test/plugins/browser_blocking.js
    //mochitests/content/browser/browser/base/content/test/plugins/browser_bug743421.js
    //mochitests/content/browser/browser/base/content/test/plugins/browser_bug787619.js
    //mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js
    //mochitests/content/browser/browser/base/content/test/plugins/browser_private_clicktoplay.js
    //mochitests/content/browser/browser/base/content/test/popupNotifications/browser_popupNotification.js
    //mochitests/content/browser/browser/base/content/test/popupNotifications/browser_popupNotification_2.js
    //mochitests/content/browser/browser/base/content/test/popupNotifications/browser_popupNotification_3.js
    //mochitests/content/browser/browser/base/content/test/popupNotifications/browser_popupNotification_4.js
    //mochitests/content/browser/browser/base/content/test/popupNotifications/browser_popupNotification_checkbox.js
    //mochitests/content/browser/browser/base/content/test/popupNotifications/browser_popupNotification_keyboard.js
    //mochitests/content/browser/browser/base/content/test/popupNotifications/browser_popupNotification_selection_required.js
    //mochitests/content/browser/browser/base/content/test/popupNotifications/head.js
    //mochitests/content/browser/browser/base/content/test/sidebar/browser_sidebar_move.js
    //mochitests/content/browser/browser/base/content/test/urlbar/browser_locationBarCommand.js
    //mochitests/content/browser/browser/base/content/test/urlbar/browser_page_action_menu.js
    //mochitests/content/browser/browser/base/content/test/urlbar/browser_page_action_menu_add_search_engine.js
    //mochitests/content/browser/browser/base/content/test/urlbar/browser_urlbarAboutHomeLoading.js
    //mochitests/content/browser/browser/base/content/test/urlbar/browser_urlbarDecode.js
    //mochitests/content/browser/browser/base/content/test/urlbar/browser_urlbarKeepStateAcrossTabSwitches.js
    //mochitests/content/browser/browser/base/content/test/urlbar/browser_urlbarOneOffs.js
    //mochitests/content/browser/browser/base/content/test/urlbar/browser_urlbar_search_speculative_connect_mousedown.js
    //mochitests/content/browser/browser/base/content/test/urlbar/head.js
    //mochitests/content/browser/browser/base/content/test/webextensions/head.js
    //mochitests/content/browser/browser/base/content/test/webrtc/browser_devices_get_user_media.js
    //mochitests/content/browser/browser/base/content/test/webrtc/browser_devices_get_user_media_anim.js
    //mochitests/content/browser/browser/base/content/test/webrtc/browser_devices_get_user_media_default_permissions.js
    //mochitests/content/browser/browser/base/content/test/webrtc/browser_devices_get_user_media_in_frame.js
    //mochitests/content/browser/browser/base/content/test/webrtc/browser_devices_get_user_media_multi_process.js
    //mochitests/content/browser/browser/base/content/test/webrtc/browser_devices_get_user_media_paused.js
    //mochitests/content/browser/browser/base/content/test/webrtc/browser_devices_get_user_media_queue_request.js
    //mochitests/content/browser/browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js
    //mochitests/content/browser/browser/base/content/test/webrtc/browser_devices_get_user_media_tear_off_tab.js
    //mochitests/content/browser/browser/base/content/test/webrtc/browser_devices_get_user_media_unprompted_access.js
    //mochitests/content/browser/browser/base/content/test/webrtc/browser_devices_get_user_media_unprompted_access_in_frame.js
    //mochitests/content/browser/browser/base/content/test/webrtc/browser_devices_get_user_media_unprompted_access_queue_request.js
    //mochitests/content/browser/browser/base/content/test/webrtc/browser_devices_get_user_media_unprompted_access_tear_off_tab.js
    //mochitests/content/browser/browser/base/content/test/webrtc/head.js
    //mochitests/content/browser/browser/components/contextualidentity/test/browser/browser_windowName.js
    //mochitests/content/browser/browser/components/customizableui/test/browser_876926_customize_mode_wrapping.js
    //mochitests/content/browser/browser/components/customizableui/test/browser_876944_customize_mode_create_destroy.js
    //mochitests/content/browser/browser/components/customizableui/test/browser_887438_currentset_shim.js
    //mochitests/content/browser/browser/components/customizableui/test/browser_918049_skipintoolbarset_dnd.js
    //mochitests/content/browser/browser/components/customizableui/test/browser_942581_unregisterArea_keeps_placements.js
    //mochitests/content/browser/browser/components/customizableui/test/browser_968565_insert_before_hidden_items.js
    //mochitests/content/browser/browser/components/customizableui/test/browser_970511_undo_restore_default.js
    //mochitests/content/browser/browser/components/customizableui/test/browser_981305_separator_insertion.js
    //mochitests/content/browser/browser/components/customizableui/test/browser_989751_subviewbutton_class.js
    //mochitests/content/browser/browser/components/customizableui/test/browser_synced_tabs_menu.js
    //mochitests/content/browser/browser/components/customizableui/test/head.js
    //mochitests/content/browser/browser/components/enterprisepolicies/tests/browser/browser_policy_search_engine.js
    //mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js
    //mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_commands_onCommand.js
    //mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_contextMenus.js
    //mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_menus.js
    //mochitests/content/browser/browser/components/extensions/test/browser/head.js
    //mochitests/content/browser/browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_popup_resize.js
    //mochitests/content/browser/browser/components/extensions/test/browser/test-oop-extensions/browser_ext_commands_onCommand.js
    //mochitests/content/browser/browser/components/extensions/test/browser/test-oop-extensions/browser_ext_contextMenus.js
    //mochitests/content/browser/browser/components/extensions/test/browser/test-oop-extensions/browser_ext_menus.js
    //mochitests/content/browser/browser/components/extensions/test/browser/test-oop-extensions/head.js
    //mochitests/content/browser/browser/components/originattributes/test/browser/browser_favicon_firstParty.js
    //mochitests/content/browser/browser/components/originattributes/test/browser/browser_favicon_userContextId.js
    //mochitests/content/browser/browser/components/places/tests/browser/browser_bookmarks_change_title.js
    //mochitests/content/browser/browser/components/places/tests/browser/browser_bug631374_tags_selector_scroll.js
    //mochitests/content/browser/browser/components/places/tests/browser/browser_check_correct_controllers.js
    //mochitests/content/browser/browser/components/places/tests/browser/browser_click_bookmarks_on_toolbar.js
    //mochitests/content/browser/browser/components/places/tests/browser/browser_drag_bookmarks_on_toolbar.js
    //mochitests/content/browser/browser/components/places/tests/browser/browser_editBookmark_tags_liveUpdate.js
    //mochitests/content/browser/browser/components/places/tests/browser/browser_panelview_bookmarks_delete.js
    //mochitests/content/browser/browser/components/places/tests/browser/browser_sidebarpanels_click.js
    //mochitests/content/browser/browser/components/places/tests/browser/browser_stayopenmenu.js
    //mochitests/content/browser/browser/components/places/tests/browser/browser_toolbar_overflow.js
    //mochitests/content/browser/browser/components/places/tests/browser/browser_views_iconsupdate.js
    //mochitests/content/browser/browser/components/places/tests/browser/browser_views_liveupdate.js
    //mochitests/content/browser/browser/components/places/tests/browser/head.js
    //mochitests/content/browser/browser/components/search/test/browser_oneOffContextMenu.js
    //mochitests/content/browser/browser/components/search/test/browser_oneOffContextMenu_setDefault.js
    //mochitests/content/browser/browser/components/search/test/browser_oneOffHeader.js
    //mochitests/content/browser/browser/components/search/test/browser_searchbar_keyboard_navigation.js
    //mochitests/content/browser/browser/components/search/test/browser_searchbar_smallpanel_keyboard_navigation.js
    //mochitests/content/browser/browser/components/search/test/browser_tooManyEnginesOffered.js
    //mochitests/content/browser/browser/components/search/test/head.js
    //mochitests/content/browser/browser/components/uitour/test/browser_UITour3.js
    //mochitests/content/browser/browser/extensions/formautofill/test/browser/head.js
    //mochitests/content/browser/browser/extensions/screenshots/test/browser/browser_screenshots_ui_check.js
    //mochitests/content/browser/browser/extensions/webcompat-reporter/test/browser/head.js
    //mochitests/content/browser/browser/modules/test/browser/browser_PageActions.js
    //mochitests/content/browser/browser/modules/test/browser/head.js
    //mochitests/content/browser/dom/html/test/browser_content_contextmenu_userinput.js
    //mochitests/content/browser/dom/indexedDB/test/head.js
    //mochitests/content/browser/dom/notification/test/browser/browser_permission_dismiss.js
    //mochitests/content/browser/dom/quota/test/head.js
    //mochitests/content/browser/dom/webauthn/tests/browser/browser_webauthn_prompts.js
    //mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_formless_submit_chrome.js
    //mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_notifications.js
    //mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_notifications_password.js
    //mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_notifications_username.js
    //mochitests/content/browser/toolkit/components/passwordmgr/test/browser/head.js
    //mochitests/content/browser/toolkit/content/tests/browser/browser_autoplay_policy_request_permission.js
    //normandy/lib/Heartbeat.jsm
    file:///builds/worker/workspace/build/application/firefox/omni.ja!/components/nsLoginManagerPrompter.js

	Brian Grinstead [:bgrins] Assignee
	Comment 32 • 6 years ago

Paolo, I've tried to make this as narrow as possible by only applying it to files that actually call these APIs from browser.xul, but I know it's still a big patch (sorry). If you are happy with the changes, I'd like to get an initial set like this landed and then spin off follow ups to fix any missing case we discover when doing more testing in browser.xhtml.

	Andreas Tolfsen ❲:ato❳
	Comment 33 • 6 years ago
mozreview-review

Comment on attachment 8996311 [details]
Bug 1479125 - Update marionette driver to access tabBrowser.tabs as an iterable;

https://reviewboard.mozilla.org/r/260460/#review269012

Sorry for intruding.  Whilst paolo is an outstanding chap, he’s not
a peer of this module.  But thanks for the patch!

	Brian Grinstead [:bgrins] Assignee
	Comment 34 • 6 years ago

(In reply to Andreas Tolfsen 「:ato」 from comment #33)
> Comment on attachment 8996311 [details]
> Bug 1479125 - Update marionette driver to access tabBrowser.tabs as an
> iterable;
> 
> https://reviewboard.mozilla.org/r/260460/#review269012
> 
> Sorry for intruding.  Whilst paolo is an outstanding chap, he’s not
> a peer of this module.  But thanks for the patch!

Thanks for the review :)

	:Paolo Amadini
	Comment 37 • 6 years ago
mozreview-review

Comment on attachment 8996350 [details]
Bug 1479125 - Migrate calls that expect an element to be returned to use element variation firstChild etc to firstElementChild etc;

https://reviewboard.mozilla.org/r/260486/#review269052

I think we will potentially see regressions only when the document is actually parsed as XHTML instead of XUL and the whitespace text nodes start to exist, but for the moment the scripted rewrite looks good to me.

The only thing that may have effects now is the childNodes->children conversion. Even though NodeList and HTMLCollection are similar, I seem to remember that in at least one case in a recent review, one of them didn't work and the other did in the same loop. Anyways, there's nothing like landing the patch to see if this still happens in some of the cases touched here :-)

Since I and probably many others would just use firstChild and other non-element variations when writing new code out of habit, I strongly recommend adding an ESLint rule that requires whitelisting each usage of the non-element functions in the affected files, with maybe the exception of "firstChild.remove" and "lastChild.remove" being called immediately. This can be done in a follow-up bug.

	Pulsebot
	Comment 38 • 6 years ago

Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0b8b186eb712
Update marionette driver to access tabBrowser.tabs as an iterable;r=ato
https://hg.mozilla.org/integration/autoland/rev/c266a7f4237e
Migrate calls that expect an element to be returned to use element variation firstChild etc to firstElementChild etc;r=Paolo

	Noemi Erli[:noemi_erli]
	Comment 39 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0b8b186eb712
https://hg.mozilla.org/mozilla-central/rev/c266a7f4237e