stylo: Gecko_AtomAttrValue usage is a bit bogus

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: bz, Assigned: bz)

Tracking

53 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

Testcase:

  <style type="text/css">
    #foo {}
  </style>
  <body id>
  </body>

This triggers: 

  ###!!! ASSERTION: wrong type: 'Type() == eAtom', file ../../../mozilla/dom/base/nsAttrValue.h, line 494

with a callstack pointing to Gecko_AtomAttrValue.

In Gecko, id attributes are stored as atoms _except_ if the value is empty; that case is stored as a string, effectively.  Now it happens to be the case that an empty string is stored as a null pointer in nsAttrValue, so when that gets returned as nsIAtom* servo sees a null there and treats that as None.

Anyway, in terms of fixes, seems like we should just check attr->Type() == nsAttrValue::eAtom and return null if so.
Created attachment 8868253 [details] [diff] [review]
Make Gecko_AtomAttrValue work correctly when the attr's value is not an atom

MozReview-Commit-ID: Btf7ZI4ZdMX
Attachment #8868253 - Flags: review?(emilio+bugs)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8868253 [details] [diff] [review]
Make Gecko_AtomAttrValue work correctly when the attr's value is not an atom

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

Thanks for catching this :)
Attachment #8868253 - Flags: review?(emilio+bugs) → review+
Well, the assert got hit a bunch when I loaded <https://addons.mozilla.org/en-US/firefox/>...  ;)

Comment 4

11 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/159f82e6813c
Make Gecko_AtomAttrValue work correctly when the attr's value is not an atom.  r=emilio

Comment 5

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/159f82e6813c
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.