950842 - CustomizableUI should reject DOM incompatible widget ID's

Status: NEW

(Firefox :: Toolbars and Customization, defect)

Description

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Currently CUI let's you register widgets with `id` that starts with number, but will throw on unregistered with "An invalid or illegal string was specified" DOMException presumably because throws by some DOM API.

Comment 1

[:Gijs (he/him)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #0)
> Currently CUI let's you register widgets with `id` that starts with number,
> but will throw on unregistered with "An invalid or illegal string was
> specified" DOMException presumably because throws by some DOM API.

We're going to need more detail than this to do something. document.getElementById("0foo") doesn't throw. What's throwing? Got a stacktrace? STR/testcase?

Comment 2

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to :Gijs Kruitbosch (Unavailable Dec 19 - Jan 2) from comment #1)
> (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> #0)
> > Currently CUI let's you register widgets with `id` that starts with number,
> > but will throw on unregistered with "An invalid or illegal string was
> > specified" DOMException presumably because throws by some DOM API.
> 
> We're going to need more detail than this to do something.
> document.getElementById("0foo") doesn't throw. What's throwing? Got a
> stacktrace? STR/testcase?

Just register widget with that Id and then unregister, you'll get a test case and
a stack trace.

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

To give more context on this here is a log from IRC conversation other day:

11:40 <gozala> jaws: Unfocused hey guys every now and then I see DOMException with message "An invalid or illegal string was specified” 
11:41 <gozala> coming from CUI, any ideas what can it be ?
11:42 ⇐ Archaeopteryx quit ([e-mail]) Quit: Goodbye
11:42 <jaws> nope :(
11:42 <mconley> gozala: where from CUI?
11:43 <mconley> gozala: STR?
11:43 <gozala> mconley: I believe when calling unregisterWidget
11:43 <gozala> it’s just logged
11:43 <mconley> gozala: hrm.
11:43 <mconley> gozala: do you have reliable STR?
11:44 ⇐ jedp quit ([e-mail]) Quit: Textual IRC Client: www.textualapp.com
11:44 <gozala> mconley: it’s also not always the case
11:44 <mconley> gozala: hrm.
11:44 <gozala> mconley: so it’s actually on add-on unload which is on FF shutdown
11:44 <mconley> ah
11:45 <gozala> mconley: on regular unload seems to work fine
11:45 <mconley> hm
11:45  → Archaeopteryx and jedp joined  •  ehsan|away → ehsan  
11:46 <gozala> mconley: actually no, I was wrong it’s not related to shutdown
11:46 <gozala> happens on add-on unload too
11:46  → Janko joined  ⇐ phlsa quit  
11:48 <gozala> mconley: it’s coming from this line it looks like https://github.com/mozilla/gecko-dev/blob/master/browser/components/customizableui/src/CustomizableUI.jsm#L1880-L1881
11:49 <mconley> interesting
11:49 <mconley> can you attach the debugger and see what aWidgetId is?
11:49 <•Mossop> What ID are you using for your widget, the DOM has specific rules about what a valid ID is
11:49 <gozala> mconley: I meant https://github.com/mozilla/gecko-dev/blob/master/browser/components/customizableui/src/CustomizableUI.jsm#L1865
11:49  → sfoster (was sfoster_) joined  ⇐ KWierso and sevaan quit  
11:50 <gozala> Mossop: it’s valid ID and CUI also validates it
11:50 <gozala> before actually creating a widget
11:50 <mconley> gozala: can you dump what aWidgetId at that point in the code?
11:51 ⇐ sfosters quit ([e-mail]) Ping timeout
11:51 <gozala> mconley: c6bfb041-a221-4e54-ab7e-f73cde123e92jetpack-frame-datatext-htmlframe-1
11:51 → KWierso joined ([e-mail])
11:51 <gozala> mconley: but as said sometimes it’s sometimes it’s not
11:52 <gozala> so there must be some race condition
11:52 <mconley> gozala: right. You might want to keep that breakpoint set, or keep dumping that value, and see if you can reproduce the issue, and then find out what that value is.
11:52 <mconley> jaws: hey - I'm thinking of doing some Holly merges. Sound OK?
11:53 <jaws> go for it!
11:53 → phlsa joined ([e-mail])
11:53 <gozala> mconley: the thing is if I console.log id before unregister it’s a lot harder to reproduce 
11:53 <mconley> :/
11:53 → sevaan joined ([e-mail])
11:55 <gozala> mconley: oh I think it maybe our fault 
11:55 <gozala> I prefix widget ID with add-on id which may start with number
11:55 <gozala> In such case all the id validation seems to pass but then it fails when querying dom
11:56 <gozala> mconley: probably worth enhancing widget ID validator to reject such ID's
11:57 <gozala> mconley: it was happening occasionally because our test runner generates test add-ons with uuid Id
11:57 ⇐ harth_ quit ([e-mail]) Input/output error
11:57 <gozala> so it failed when uuid wast starting with number
11:57 <mconley> hmmm
11:57 <mconley> gozala: yes, we should probably reject widgets attempting to register with IDs that are not DOM id compatible
11:57 <mconley> gozala: could you please file a bug blocking australis-cust?
11:58 <•Mossop> gozala: See bug 923362
11:58 <firebot> Bug https://bugzilla.mozilla.org/show_bug.cgi?id=923362 nor, P1, ---, nobody, NEW, Button tries to use an invalid ID if your add-on ID contains a period (.)
11:58 <•Mossop> Oh I guess that's slightly different
11:58 <gozala> Mossop: I do normalize for that
11:58 <gozala> mconley: sure I’ll do that
11:58 <mconley> gozala: cool, thanks


That being said I have tried this:

let id = "6cbfb041-a221-4e54-ab7e-f73cde123e92jetpack-frame-datatext-htmlframe-1";
CustomizableUI.createWidget({
  id: id,
  defaultArea: CustomizableUI.AREA_NAVBAR,
  onBuild: document => {
    let node = document.createElement("toolbaritem");
    node.id = id;
    return node;
  }
})
CustomizableUI.destroyWidget(id)

Which strangely enough doesn't seem to reproduce issue, I suspect that combination of toolbar + widget
maybe causing it.

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Ok so if you wanna reproduce this bug just run following in scratchpad with browser context:

let id = "950842-is-bug";
CustomizableUI.createWidget({
  id: id,
  defaultArea: CustomizableUI.AREA_NAVBAR,
  onBuild: document => {
    let node = document.createElement("toolbaritem");
    node.id = id;
    return node;
  }
})
window.open()


Opening new window causes error and also messes up CUI.