Broken menu: NS_ERROR_ILLEGAL_VALUE: id, attribute, or value too long

VERIFIED FIXED in Firefox 34

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: soeren.hentzschel, Assigned: rvitillo)

Tracking

({regression})

Trunk
Firefox 35
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox32 unaffected, firefox33 unaffected, firefox34+ verified, firefox35 verified)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Reporter

Description

5 years ago
Posted image screenshot
I can't reproduce the issue with a clean profile, but it's reproducible with my daily profile. Nevertheless, I think the menu shouldn't become unusable, even in a "broken profile". ;-)

STR:

0. There is an icon on the right side in the tab strip.
1. open customize mode
2. move the icon a few pixels to the right and back to the left
3. leave the customize mode

Expected:

Nothing changed.

Actual:

The menu can no longer be used, see the screenshot. Firefox has to be restarted. There is an error in the browser console:

"[CustomizeMode]" XPCWrappedNative_NoHelper { message: "id, attribute, or value too long", result: 2147942487, name: "NS_ERROR_ILLEGAL_VALUE", filename: "resource://gre/components/XULStore.js", lineNumber: 211, columnNumber: 0, location: XPCWrappedNative_NoHelper, inner: null, data: null } CustomizeMode.jsm:56
NS_ERROR_ILLEGAL_VALUE: id, attribute, or value too long
[Tracking Requested - why for this release]:
Blocks: 559505
Keywords: regression
Assignee: nobody → rvitillo
Any good reason why we should keep that test?
Attachment #8477375 - Flags: review?(enndeakin)
(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #2)
> Created attachment 8477375 [details] [diff] [review]
> Broken menu: NS_ERROR_ILLEGAL_VALUE: id, attribute, or value too long, v1
> 
> Any good reason why we should keep that test?

Keeping the cap seems reasonable (it doesn't make sense to store more data than that). Hitting the cap shouldn't break anyone's menus, though - it seems like the thing to fix is not dealing with that error case correctly somewhere?

Comment 4

5 years ago
Comment on attachment 8477375 [details] [diff] [review]
Broken menu: NS_ERROR_ILLEGAL_VALUE: id, attribute, or value too long, v1

Yes, I think we should figure out why this breaks the menu here, as this check was being done before with the old code as well.

It may require more data about what is in the profile that causes this though. For example, by making a copy and reducing it to find out what specifically is causing the problem.
Attachment #8477375 - Flags: review?(enndeakin) → review-
I can't reproduce it with my profile. Sören could you please follow up with the information requested by Neil? Thanks!
Flags: needinfo?(cadeyrn)
Reporter

Comment 6

5 years ago
(In reply to Neil Deakin from comment #4)
> It may require more data about what is in the profile that causes this
> though. For example, by making a copy and reducing it to find out what
> specifically is causing the problem.

I copied my profile and uninstalled all add-ons in the copied profile, the problem still exists. What should I test next?
Flags: needinfo?(cadeyrn)

Comment 7

5 years ago
(In reply to Sören Hentzschel from comment #6)
> (In reply to Neil Deakin from comment #4)
> > It may require more data about what is in the profile that causes this
> > though. For example, by making a copy and reducing it to find out what
> > specifically is causing the problem.
> 
> I copied my profile and uninstalled all add-ons in the copied profile, the
> problem still exists. What should I test next?

Can you provide a copy of the xulstore and localstore.rdf files?
Reporter

Comment 8

5 years ago
Posted file localstore.rdf
Reporter

Comment 9

5 years ago
Posted file xulstore.json

Comment 10

5 years ago
Thanks, Sören!

(In reply to Neil Deakin from comment #4)
> Comment on attachment 8477375 [details] [diff] [review]
> Broken menu: NS_ERROR_ILLEGAL_VALUE: id, attribute, or value too long, v1
> 
> Yes, I think we should figure out why this breaks the menu here, as this
> check was being done before with the old code as well.

That's odd, because from the attached localstore.rdf, the value for the nav-bar's currentset:

"unified-back-forward-button,urlbar-container,reload-button,stop-button,search-container,webrtc-status-button,bookmarks-menu-button,downloads-button,action-button--jid1-ulb4rmb4wb5efwjetpack-my-button,sync-button,button--soundcloud-playermikedeboernl-soundcloud-player,greasemonkey-tbb,scriptish-button,button--aboutconfigbuttonagenediacom-copy-extensions,social-status-button-talkilla-mozillalabs-com,social-mark-button-getpocket-com,RIL_toolbar_button,action-button--jid1-tdcgyn3laa05kqjetpack-test-2,action-button--jid1-tdcgyn3laa05kqjetpack-test-3,action-button--jid1-tdcgyn3laa05kqjetpack-test-4,action-button--jid1-tdcgyn3laa05kqjetpack-test-44,action-button--jid1-tdcgyn3laa05kqjetpack-test-5,action-button--jid1-tdcgyn3laa05kqjetpack-test-6,action-button--jid1-tdcgyn3laa05kqjetpack-test-7,social-status-button-mixedpuppy-github-io,widget:jid1-WwVLR26pEIqm1A@jetpack-currentpushlog,button--jid1-ulb4rmb4wb5efwjetpack-my-button,bluhellfirewall-toolbar-button,noscript-tbb,noscript-tbb-revoke-temp,noscript-tbb-temp-page,action-button--jid1-yayz8qxoulbgtwjetpack-copy-extensions,useragentoverrider-button,loop-call-button".length
1126
When I use the attached locastore.rdf I get some errors during the importing step since there is at least one attribute value with a length greater than 1024 (see comment 10). It looks like the former code had a threshold of 4096 for attribute values (http://hg.mozilla.org/mozilla-central/file/96a566fa1599/content/xul/document/src/XULDocument.cpp#l1387) and 512 for attribute names (http://hg.mozilla.org/mozilla-central/file/96a566fa1599/content/xul/document/src/XULDocument.cpp#l1370). In particular, for attribute values, the value was truncated and no error was raised. I would prefer not to truncate and yield an error instead as it seems cleaner to me.
Attachment #8477375 - Attachment is obsolete: true
Attachment #8479898 - Flags: review?(enndeakin)

Comment 13

5 years ago
Comment on attachment 8479898 [details] [diff] [review]
Broken menu: NS_ERROR_ILLEGAL_VALUE: id, attribute, or value too long, v2

This doesn't fix the bug, since someone could possibly have a value larger than 4096 as well. Does the old code fail and cause this bug in this case?
No it doesn't as I mentioned in comment 11: it truncates the value. But do we really want to keep the same behavior of storing an incomplete value? Shouldn't the callers know that an incomplete value has been stored and handle it accordingly?
My last sentence should have been: shouldn't the callers know that the value is too long to be stored?
Flags: needinfo?(enndeakin)
It doesn't feel clean truncating values but it's backwards compatible to what we had before.
Attachment #8479898 - Attachment is obsolete: true
Attachment #8479898 - Flags: review?(enndeakin)
Attachment #8481294 - Flags: review?(enndeakin)
Flags: needinfo?(enndeakin)

Updated

5 years ago
Attachment #8481294 - Flags: review?(enndeakin) → review+

Comment 19

5 years ago
https://hg.mozilla.org/mozilla-central/rev/7da521607978
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
This bug has been marked as impacting 34 (aurora). As the fix has been on m-c for a while, can you please submit an aurora approval request?
Flags: needinfo?(rvitillo)
Comment on attachment 8481294 [details] [diff] [review]
NS_ERROR_ILLEGAL_VALUE: id, attribute, or value too long, v3

Review of attachment 8481294 [details] [diff] [review]:
-----------------------------------------------------------------

Approval Request Comment

[Feature/regressing bug #]:  Bug 1057137 - Broken menu: NS_ERROR_ILLEGAL_VALUE: id, attribute, or value too long
[User impact if declined]: Menus might break.
[Describe test coverage new/current, TBPL]: Tests passed https://tbpl.mozilla.org/?tree=Try&rev=a196a756d69a.
[Risks and why]: No risks; tests pass.
[String/UUID change made/needed]: None.
Attachment #8481294 - Flags: approval-mozilla-aurora?
Flags: needinfo?(rvitillo)
Comment on attachment 8481294 [details] [diff] [review]
NS_ERROR_ILLEGAL_VALUE: id, attribute, or value too long, v3

(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #21)
> [Feature/regressing bug #]:  Bug 1057137 - Broken menu:
> NS_ERROR_ILLEGAL_VALUE: id, attribute, or value too long

Bug 1057137 is this bug. Do you know in what bug the issue was introduced? Or, how far back does this issue go?
Attachment #8481294 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(rvitillo)
Lawrence, the bug was introduced in Bug 559505 when I replaced localstore with xulstore.
Flags: needinfo?(rvitillo)
Thanks Roberto. As bug 559505 landed in 34, this fix only needs to go as far back as Aurora.
Soren, can you confirm this fix in:
- Firefox 34 Beta 1 (or more recent) - ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/34.0b1-candidates/build2/
and/or
- latest Firefox Aurora - ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-aurora/
Flags: needinfo?(cadeyrn)
Reporter

Comment 27

5 years ago
Sorry for the delay. It seems to be fixed in latest Beta and Aurora, thanks.
Status: RESOLVED → VERIFIED
Flags: needinfo?(cadeyrn)
You need to log in before you can comment on or make changes to this bug.