Rewrite callers firstChild/lastChild/childNodes/previousSibling/nextSibling with firstElementChild/lastElementChild/children/previousElementSibling/nextElementSibling in browser.xul

RESOLVED FIXED in Firefox 63

Status

()

P3
normal
RESOLVED FIXED
8 months ago
5 months ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

8 months 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.
(Assignee)

Updated

8 months ago
Summary: Rewrite callers firstChild/lastChild/childNodes with firstElementChild/lastElementChild/children in browser.xul → Rewrite callers firstChild/lastChild/childNodes/previousSibling/nextSibling with firstElementChild/lastElementChild/children/previousElementSibling/nextElementSibling in browser.xul
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

8 months 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.
(Assignee)

Comment 6

8 months 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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

8 months 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.
(Assignee)

Updated

8 months ago
Depends on: 1473165
(Assignee)

Updated

8 months ago
Blocks: 1473165
No longer depends on: 1473165
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

8 months 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.
(Assignee)

Comment 15

8 months ago
Splitting the logging patch out from mozreview
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8996351 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
(Assignee)

Comment 18

8 months 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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8998573 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 32

8 months 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.

Comment 33

8 months 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!
Attachment #8996311 - Flags: review+
(Assignee)

Comment 34

8 months 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 :)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 37

8 months 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.
Attachment #8996350 - Flags: review?(paolo.mozmail) → review+

Comment 38

8 months 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

Comment 39

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

Updated

7 months ago
Blocks: 1482667

Updated

5 months ago
Depends on: 1503342
You need to log in before you can comment on or make changes to this bug.