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

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Sören Hentzschel]

Attachment: 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

Comment 1

[Dão Gottwald [:dao]]

[Tracking Requested - why for this release]:

Comment 2

[Roberto Agostino Vitillo (:rvitillo)]

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

Any good reason why we should keep that test?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

(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

[Neil Deakin]

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.

Comment 5

[Roberto Agostino Vitillo (:rvitillo)]

I can't reproduce it with my profile. Sören could you please follow up with the information requested by Neil? Thanks!

Comment 6

[Sören Hentzschel]

(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?

Comment 7

[:Gijs (he/him)]

(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?

Comment 8

[Sören Hentzschel]

Attachment: localstore.rdf

Comment 9

[Sören Hentzschel]

Attachment: xulstore.json

Comment 10

[:Gijs (he/him)]

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

Comment 11

[Roberto Agostino Vitillo (:rvitillo)]

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.

Comment 12

[Roberto Agostino Vitillo (:rvitillo)]

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

Comment 13

[Neil Deakin]

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?

Comment 14

[Roberto Agostino Vitillo (:rvitillo)]

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?

Comment 15

[Roberto Agostino Vitillo (:rvitillo)]

My last sentence should have been: shouldn't the callers know that the value is too long to be stored?

Comment 16

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: NS_ERROR_ILLEGAL_VALUE: id, attribute, or value too long, v3

It doesn't feel clean truncating values but it's backwards compatible to what we had before.

Comment 17

[Roberto Agostino Vitillo (:rvitillo)]

https://tbpl.mozilla.org/?tree=Try&rev=a196a756d69a

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/7da521607978

Comment 19

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/7da521607978

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 21

[Roberto Agostino Vitillo (: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.

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 23

[Roberto Agostino Vitillo (:rvitillo)]

Lawrence, the bug was introduced in Bug 559505 when I replaced localstore with xulstore.

Comment 24

[Lawrence Mandel [:lmandel] (use needinfo)]

Thanks Roberto. As bug 559505 landed in 34, this fix only needs to go as far back as Aurora.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/098e2b50221b

Comment 26

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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/

Comment 27

[Sören Hentzschel]

Sorry for the delay. It seems to be fixed in latest Beta and Aurora, thanks.