Open
Bug 950842
Opened 10 years ago
Updated 1 year ago
CustomizableUI should reject DOM incompatible widget ID's
Categories
(Firefox :: Toolbars and Customization, defect)
Firefox
Toolbars and Customization
Tracking
()
NEW
People
(Reporter: irakli, Unassigned)
References
(Blocks 1 open bug)
Details
(Whiteboard: [Australis:P5])
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.
Reporter | ||
Updated•10 years ago
|
Blocks: australis-cust
Comment 1•10 years ago
|
||
(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?
Flags: needinfo?(rFobic)
OS: Mac OS X → All
Hardware: x86 → All
Version: 26 Branch → Trunk
Reporter | ||
Comment 2•10 years ago
|
||
(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.
Flags: needinfo?(rFobic)
Reporter | ||
Comment 3•10 years ago
|
||
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 (itsme@moz-AC58209D.cust.telecolumbus.net) 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 (jedp@moz-89599B04.dsl.dynamic.sonic.net) 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 (sfoster@moz-99C13B58.hsd1.or.comcast.net) Ping timeout 11:51 <gozala> mconley: c6bfb041-a221-4e54-ab7e-f73cde123e92jetpack-frame-datatext-htmlframe-1 11:51 → KWierso joined (chatzilla@moz-3D85277A.hsd1.ca.comcast.net) 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 (phlsa@moz-97BEEEBA.dip0.t-ipconnect.de) 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 (sevaan@13F2CEC5.7672369.D8E68FF6.IP) 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 (harth@moz-42412102.hsd1.ca.comcast.net) 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.
Reporter | ||
Comment 4•10 years ago
|
||
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.
Updated•10 years ago
|
Whiteboard: [Australis:P5]
Updated•1 year ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•