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

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Carsten Book [:Tomcat]]

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

Comment 1

[Carsten Book [:Tomcat]]

[Tracking Requested - why for this release]:

bughunter live website result

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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

Comment 3

[Astley Chen (inactive)]

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.

Comment 4

[Timothy Nikkel (:tnikkel)]

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.

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: cleanup: Rename a variable to be slightly clearer

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: disallow anchors

Comment 7

[Timothy Nikkel (:tnikkel)]

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 8

[Mats Palmgren (inactive)]

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

Comment 9

[Mats Palmgren (inactive)]

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...

Comment 10

[Timothy Nikkel (: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.

Comment 11

[Mats Palmgren (inactive)]

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

Comment 12

[Timothy Nikkel (: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.

Comment 13

[Mats Palmgren (inactive)]

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

Comment 14

[Gerry Chang [:gchang]]

Track 51- as this only happened in debug builds and mark 51 fix-optional. Still happy to take the fix in 51 early beta.

Comment 15

[Julien Cristau [:jcristau]]

If this depends on a 15 year old bug, I guess I shouldn't hold my breath... Not tracking for 52/53.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Would be nice to add a negative test for this.

Comment 17

[Mats Palmgren (inactive)]

Comment on attachment 8812397 [details] [diff] [review]
disallow anchors

r- per comment 13.

Comment 18

[Timothy Nikkel (:tnikkel)]

Attachment: disallow anchors

Comment 19

[Timothy Nikkel (:tnikkel)]

Attachment: mochitest

Comment 20

[Mats Palmgren (inactive)]

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.

Comment 21

[Timothy Nikkel (:tnikkel)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9418529677baa82ad9dee34b5370ed556da64ab

Comment 22

[Timothy Nikkel (:tnikkel)]

(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

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Did this land? If not, is there something blocking it? Same for bug 1323914.

Comment 24

[Timothy Nikkel (:tnikkel)]

Hasn't landed yet. I wanted to send an intent to unship but haven't gotten around to it yet.

Comment 26

[Mats Palmgren (inactive)]

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.

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1317937: Rename nsImageMap::mContainsBlockContents to mConsiderWholeSubtree so it is more descriptive of what it actually does.

Review commit: https://reviewboard.mozilla.org/r/197684/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197684/

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1317937: Disallow <a> as <area> in image maps.

Review commit: https://reviewboard.mozilla.org/r/197686/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197686/

Comment 29

[Emilio Cobos Álvarez (:emilio)]

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

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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

[Mats Palmgren (inactive)]

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

Comment 32

[Mats Palmgren (inactive)]

Comment on attachment 8926439 [details]
Bug 1317937: Disallow <a> as <area> in image maps.

https://reviewboard.mozilla.org/r/197686/#review202930

Comment 33

[Mats Palmgren (inactive)]

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.

Comment 34

[Emilio Cobos Álvarez (:emilio)]

(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

Comment 35

[Pulsebot]

Pushed by [e-mail]:
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

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/33824395a6ca
https://hg.mozilla.org/mozilla-central/rev/0f4264c81857

Comment 37

[Kohei Yoshino [:kohei]]

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/a-elements-can-no-longer-be-used-as-image-map-regions/

Comment 38

[Chris Mills [:cmills]]

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

Comment 39

[Emilio Cobos Álvarez (:emilio)]

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

Comment 40

[Chris Mills [:cmills]]

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