Stop supporting <a> as <area> in image maps (Assertion failure: !aContent->GetPrimaryFrame() || aState.mCreatingExtraFrames || aContent->NodeInfo()->NameAtom() == nsGkAtoms::area, at nsCSSFrameConstructor.cpp:5687)

RESOLVED FIXED in Firefox 58

Status

()

Core
Layout
P3
normal
RESOLVED FIXED
a year ago
2 days ago

People

(Reporter: Tomcat, Assigned: emilio)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, 4 keywords)

unspecified
mozilla58
assertion, dev-doc-complete, html5, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51- wontfix, firefox52- wontfix, firefox-esr52 wontfix, firefox53- wontfix, firefox56 wontfix, firefox57 wontfix, firefox58 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
Created attachment 8811187 [details]
stack

Found by bughunter and reproduced on latest windows trunk debug tinderbox build.

Steps to reproduce:
-> Load http://www.zjks.com/default.aspx
(note crashed on load after ~ 5 to 10 seconds)
--> Assertion failure: !aContent->GetPrimaryFrame() || aState.mCreatingExtraFrames || aContent->NodeInfo()->NameAtom() == nsGkAtoms::area, at c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/layout/base/nsCSSFrameConstructor.cpp:5687

seems affecting trunk to beta debug builds on windows and linux
(Reporter)

Comment 1

a year ago
[Tracking Requested - why for this release]:

bughunter live website result
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
Priority: -- → P1
I use rr to dig a bit. When the assertion occurs !aContent->GetPrimaryFrame() is false, i.e. aContent already has an ImageFrame: 

ImageFrame(img)(1)@7f25a4134348 {0,0,12360,10620} [state=001040c800300200] [content=7f25a4241500] [sc=7f25a4134220^7f25a4131a28^7f25a41314f8^7f25a41315a0] [src=http://www.zjks.com/ZjImg/kjgn01.gif]

By running backwards, it turns out the frame was set in nsImageMap::AddArea [1], so this bug is probably related to bug 135040.

[1] http://searchfox.org/mozilla-central/rev/62db1c9021cfbde9fa5e6e9601de16c21f4c7ce4/layout/generic/nsImageMap.cpp#861-866
Depends on: 135040
Thanks, TY. We probably need a test case here and also revamp the tricky design in bug 135040. As it happens on debug only, lower to P3.
Priority: P1 → P3
This is because nsImageMap::SearchForAreas will consider <a> elements in place of <area> elements if it finds them first. It looks like this ability was added in 1999 in this commit http://searchfox.org/mozilla-central/commit/cb48e4047a3bfe163616415b4109db0275ec8dd9 . nsCSSFrameConstructor assumes only <area> get this special processing.

As far as I can tell, we are the only ones to implement this. The spec allowing this is only HTML 4. HTML 5 and HTML 3 both only make <area> elements inside map special, and not <a>. I tested current Safari (on mac), current Chrome (on mac), IE 11 (on Windows 8.1) in Edge mode, as well as enabling every compatibility mode it had (back to IE5). None of them supported <a> inside <map>. So as far as I can tell this was something added to HTML 4 that never caught on.

I think we should remove support for it.
Created attachment 8812396 [details] [diff] [review]
cleanup: Rename a variable to be slightly clearer
Assignee: nobody → tnikkel
Attachment #8812396 - Flags: review?(mats)
Created attachment 8812397 [details] [diff] [review]
disallow anchors
Attachment #8812397 - Flags: review?(mats)
Attachment #8812396 - Attachment description: patch → cleanup: Rename a variable to be slightly clearer
Making the change conditional on a hidden pref just in case there is some fallout from this (so we can just flip a pref to back this out). Hopefully we can just remove this code when we are confident that it'll be fine.
Comment on attachment 8812396 [details] [diff] [review]
cleanup: Rename a variable to be slightly clearer

> +  // This is set when we search for all area children and tells us whether we

nit: this could be a /** comment I guess
Attachment #8812396 - Flags: review?(mats) → review+
Comment on attachment 8812397 [details] [diff] [review]
disallow anchors

Looks OK, but does those emergency updates that just flip a pref
always require a restart?  If they are (or can be) restart-less
then this should probably use AddBoolVarCache instead...
Flags: needinfo?(tnikkel)
(In reply to Mats Palmgren (:mats) from comment #9)
> Comment on attachment 8812397 [details] [diff] [review]
> disallow anchors
> 
> Looks OK, but does those emergency updates that just flip a pref
> always require a restart?  If they are (or can be) restart-less
> then this should probably use AddBoolVarCache instead...

I specifically did not want to make this pref "live". Say a <map> subtree is modified after the preference is changed then we update the image map using different rules then other image maps on the page.
Flags: needinfo?(tnikkel)
(In reply to Timothy Nikkel (:tnikkel) from comment #10)
> Say a <map> subtree is
> modified after the preference is changed then we update the image map using
> different rules then other image maps on the page.

I don't see what the problem with that would be.  The user always needs to
reload the page to have the pref change take effect anyway.  It's still
better than forcing the user to restart IMO.

Anyway, perhaps we should start by finding out if these emergency pref-change
updates requires a restart or not?  If they do, then the issue is moot.
Flags: needinfo?(tnikkel)
(In reply to Mats Palmgren (:mats) from comment #11)
> (In reply to Timothy Nikkel (:tnikkel) from comment #10)
> > Say a <map> subtree is
> > modified after the preference is changed then we update the image map using
> > different rules then other image maps on the page.
> 
> I don't see what the problem with that would be.  The user always needs to
> reload the page to have the pref change take effect anyway.  It's still
> better than forcing the user to restart IMO.

But in this scenario, without reloading the page, we'd have some DOM subtrees handled under one set of rules, and other DOM subtrees handled under other rules in the same page at the same time.

It seems like the odds of this causing problems that are so severe we want to do an emergency update (and not just a regular update) but problems that we also somehow miss for a full nightly, aurora, beta cycle is very low.
Flags: needinfo?(tnikkel)
(In reply to Timothy Nikkel (:tnikkel) from comment #12)
> But in this scenario, without reloading the page, we'd have some DOM
> subtrees handled under one set of rules, and other DOM subtrees handled
> under other rules in the same page at the same time.

I don't see a problem with that.

> It seems like the odds of this causing problems that are so severe we want
> to do an emergency update (and not just a regular update) but problems that
> we also somehow miss for a full nightly, aurora, beta cycle is very low.

I agree, but the purpose of the pref is to be able to enable it
in an emergency update, right?  Otherwise you might as well just
remove the code.  Then, if it causes problems, we'll just revert
that and ship a regular update.  As you say, the risk of that is
extremely low.
Track 51- as this only happened in debug builds and mark 51 fix-optional. Still happy to take the fix in 51 early beta.
status-firefox51: affected → fix-optional
tracking-firefox51: ? → -
If this depends on a 15 year old bug, I guess I shouldn't hold my breath... Not tracking for 52/53.
status-firefox52: affected → fix-optional
status-firefox53: affected → fix-optional
tracking-firefox52: ? → -
tracking-firefox53: ? → -
Would be nice to add a negative test for this.
Comment on attachment 8812397 [details] [diff] [review]
disallow anchors

r- per comment 13.
Attachment #8812397 - Flags: review?(mats) → review-
Created attachment 8818818 [details] [diff] [review]
disallow anchors
Attachment #8812397 - Attachment is obsolete: true
Attachment #8818818 - Flags: review?(mats)
Created attachment 8818819 [details] [diff] [review]
mochitest

Comment 20

11 months ago
Comment on attachment 8818818 [details] [diff] [review]
disallow anchors

r=mats

BTW, it looks like AddArea is actually infallible nowadays:
https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/layout/generic/nsImageMap.cpp#818
which makes SearchForAreas infallible too.
Please file a follow-up bug on cleaning that up.
Attachment #8818818 - Flags: review?(mats) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9418529677baa82ad9dee34b5370ed556da64ab

Updated

11 months ago
Summary: Assertion failure: !aContent->GetPrimaryFrame() || aState.mCreatingExtraFrames || aContent->NodeInfo()->NameAtom() == nsGkAtoms::area, at nsCSSFrameConstructor.cpp:5687 → Stop supporting <a> as <area> in image maps (Assertion failure: !aContent->GetPrimaryFrame() || aState.mCreatingExtraFrames || aContent->NodeInfo()->NameAtom() == nsGkAtoms::area, at nsCSSFrameConstructor.cpp:5687)
(In reply to Mats Palmgren (:mats) from comment #20)
> BTW, it looks like AddArea is actually infallible nowadays:
> https://dxr.mozilla.org/mozilla-central/rev/
> 8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/layout/generic/nsImageMap.cpp#818
> which makes SearchForAreas infallible too.
> Please file a follow-up bug on cleaning that up.

follow up -> bug 1323914
(Assignee)

Comment 23

9 months ago
Did this land? If not, is there something blocking it? Same for bug 1323914.
Flags: needinfo?(tnikkel)
Hasn't landed yet. I wanted to send an intent to unship but haven't gotten around to it yet.
Blocks: 1346779
Depends on: 1350532

Updated

2 months ago
Duplicate of this bug: 1171935

Updated

20 days ago
Blocks: 135040
No longer depends on: 135040
Assuming comment 4 is right that Firefox is the only browser supporting
"<a> as <area>" then I think it's safe to remove it without getting
telemetry first.  I suggest we just rebase the patches and land them
as is.

Technically, this support is a HTML spec violation:
https://html.spec.whatwg.org/multipage/image-maps.html#image-map-processing-model
so the arguments for removing it are strong.
Keywords: html5
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 29

14 days ago
(In reply to Mats Palmgren (:mats) from comment #26)
> Assuming comment 4 is right that Firefox is the only browser supporting
> "<a> as <area>" then I think it's safe to remove it without getting
> telemetry first.  I suggest we just rebase the patches and land them
> as is.
> 
> Technically, this support is a HTML spec violation:
> https://html.spec.whatwg.org/multipage/image-maps.html#image-map-processing-
> model
> so the arguments for removing it are strong.

I talked with Timothy and he was fine with me rebasing his patches and landing this, so will do that.
Assignee: tnikkel → emilio
Flags: needinfo?(tnikkel)
(In reply to Mats Palmgren (:mats) from comment #26)
> Technically, this support is a HTML spec violation:
> https://html.spec.whatwg.org/multipage/image-maps.html#image-map-processing-
> model
> so the arguments for removing it are strong.

Although this was a nice feature, and was in HTML 4.  It allowed image maps to use visible links as the links rather than hidden ones.  But nobody else implemented it...

Comment 31

14 days ago
mozreview-review
Comment on attachment 8926438 [details]
Bug 1317937: Rename nsImageMap::mContainsBlockContents to mConsiderWholeSubtree so it is more descriptive of what it actually does.

https://reviewboard.mozilla.org/r/197684/#review202928
Attachment #8926438 - Flags: review?(mats) → review+

Comment 32

14 days ago
mozreview-review
Comment on attachment 8926439 [details]
Bug 1317937: Disallow <a> as <area> in image maps.

https://reviewboard.mozilla.org/r/197686/#review202930
Attachment #8926439 - Flags: review?(mats) → review+
Perhaps we should post an "intent to unship" on dev.platform for this,
since we're deprecating an existing "feature"?  Specifically mentioning
that we think it's OK to remove it because no other browser implemented
this HTML4 feature and that it has been removed in HTML5.
(Assignee)

Comment 34

14 days ago
(In reply to Mats Palmgren (:mats) from comment #33)
> Perhaps we should post an "intent to unship" on dev.platform for this,
> since we're deprecating an existing "feature"?  Specifically mentioning
> that we think it's OK to remove it because no other browser implemented
> this HTML4 feature and that it has been removed in HTML5.

Agreed, I posted https://groups.google.com/d/msg/mozilla.dev.platform/JUB5K-sz6ek/F4hQWdDRBQAJ

Updated

14 days ago
Keywords: dev-doc-needed, site-compat

Comment 35

13 days ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/33824395a6ca
Rename nsImageMap::mContainsBlockContents to mConsiderWholeSubtree so it is more descriptive of what it actually does. r=mats
https://hg.mozilla.org/integration/autoland/rev/0f4264c81857
Disallow <a> as <area> in image maps. r=mats

Comment 36

13 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/33824395a6ca
https://hg.mozilla.org/mozilla-central/rev/0f4264c81857
Status: NEW → RESOLVED
Last Resolved: 13 days ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/a-elements-can-no-longer-be-used-as-image-map-regions/
status-firefox51: fix-optional → wontfix
status-firefox52: fix-optional → wontfix
status-firefox53: fix-optional → wontfix
status-firefox56: --- → wontfix
status-firefox57: --- → wontfix
status-firefox-esr52: --- → wontfix
I've documented this on MDN by adding a note to the Fx58 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/58#HTML_2

I couldn't find an explicit mention of it anywhere, although the <a> page does list support for the coords and shape attributes:

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#Browser_compatibility

Should I also change these to say "no support"?
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 39

7 days ago
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #38)
> I couldn't find an explicit mention of it anywhere, although the <a> page
> does list support for the coords and shape attributes:
> 
> https://developer.mozilla.org/en-US/docs/Web/HTML/Element/
> a#Browser_compatibility
> 
> Should I also change these to say "no support"?

Yeah, that sounds reasonable to me.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #39)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #38)
> > I couldn't find an explicit mention of it anywhere, although the <a> page
> > does list support for the coords and shape attributes:
> > 
> > https://developer.mozilla.org/en-US/docs/Web/HTML/Element/
> > a#Browser_compatibility
> > 
> > Should I also change these to say "no support"?
> 
> Yeah, that sounds reasonable to me.

OK, I've submitted a pull request to our browser compat data repo to get this support info updated.
You need to log in before you can comment on or make changes to this bug.