1019319 - Firefox can no longer load Chromium Dashboard after HTML Imports landed

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Chris Peterson [:cpeterson]]

This is a regression in Nightly 32 build 2014-05-22. I bisected mozilla-inbound builds to the following pushlog, which blames HTML imports bug 877072:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=bfc1b283e10d&tochange=25763b7fe938

Curiously, I see the following JS errors in the web console when the dashboard page loads successfully in older versions of Firefox:

"Script contained non-latin characters that were forced to latin. Some characters may be wrong." Object { impl: <script>, parentNode_: undefined, firstChild_: undefined, lastChild_: undefined, nextSibling_: undefined, previousSibling_: undefined, treeScope_: Object, __importParsed: true } platform.js:15
TypeError: mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create platform.js:15
"Script contained non-latin characters that were forced to latin. Some characters may be wrong." Object { impl: <script>, parentNode_: undefined, firstChild_: undefined, lastChild_: undefined, nextSibling_: undefined, previousSibling_: undefined, treeScope_: Object, __importParsed: true } platform.js:15
"Exception caught during observer callback: .classList@http://www.chromestatus.com/static/bower_components/platform/platform.js:13:20925
.targetChanged@data:text/javascript;base64,KGZ1bmN0aW9uKCl7dmFyIG92ZXJsYXlzPVtdO3ZhciB0cmFja092ZXJsYXlzPWZ1bmN0aW9uKGluT3ZlcmxheSl7aWYoaW5PdmVybGF5Lm9wZW5lZCl7dmFyIHowPWN1cnJlbnRPdmVybGF5WigpO292ZXJsYXlzLnB1c2goaW5PdmVybGF5KTt2YXIgejE9Y3VycmVudE92ZXJsYXlaKCk7aWYoejE8PXowKXthcHBseU92ZXJsYXlaKGluT3ZlcmxheSx6MCl9fWVsc2V7dmFyIGk9b3ZlcmxheXMuaW5kZXhPZihpbk92ZXJsYXkpO2lmKGk+PTApe292ZXJsYXlzLnNwbGljZShpLDEpO3NldFooaW5PdmVybGF5LG51bGwpfX19O3ZhciBhcHBseU92ZXJsYXlaPWZ1bmN0aW9uKGluT3ZlcmxheSxpbkFib3ZlWil7c2V0Wihpbk92ZXJsYXkudGFyZ2V0LGluQWJvdmVaKzIpfTt2YXIgc2V0Wj1mdW5jdGlvbihpbk5vZGUsaW5aKXtpbk5vZGUuc3R5bGUuekluZGV4PWluWn07dmFyIGN1cnJlbnRPdmVybGF5PWZ1bmN0aW9uKCl7cmV0dXJuIG92ZXJsYXlzW292ZXJsYXlzLmxlbmd0aC0xXX07dmFyIERFRkFVTFRfWj0xMDt2YXIgY3VycmVudE92ZXJsYXlaPWZ1bmN0aW9uKCl7dmFyIHo7dmFyIGN1cnJlbnQ9Y3VycmVudE92ZXJsYXkoKTtpZihjdXJyZW50KXt2YXIgejE9d2luZG93LmdldENvbXB1d"[…] platform.js:12

Comment 1

[Gabor Krizsanits (INACTIVE)]

This is really weird... I mean I totally get why that page would be broken with the dom.webcomponents.enabled set to true... but not without. Without that my patch does not do much... Chris, are you sure you have not bisected with the flag turned on accidentally?

Comment 2

[Gabor Krizsanits (INACTIVE)]

Actually, you can detect the import property in the HTMLLinkElement, and then assume we support imports natively already, and then of course since it's turned off and not fully implemented anyway things will break...

Can we somehow lie about HTMLLinkElement implementing the import property? Like if dom.webcomponents.enabled imports is not a property, if it's enabled than it is.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It's not even lying, that property should definitely be controlled by the pref too.  The [Pref] annotation on a property in WebIDL does exactly this.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Er, yes.  I thought that this was enabled by default when I was reviewing bug 877072.

If it's not, then we need to mark the attribute with [Pref] in the IDL accordingly.

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #1)
> This is really weird... I mean I totally get why that page would be broken
> with the dom.webcomponents.enabled set to true... but not without. Without
> that my patch does not do much... Chris, are you sure you have not bisected
> with the flag turned on accidentally?

I bisected this bug (a second time) with a clean profile (so I am testing with the default value of dom.webcomponents.enabled) and found the same regression from bug 877072.

btw, I forgot to include the Chromium Dashboard link for anyone not familiar with it:
http://www.chromestatus.com/features

Comment 6

[Gabor Krizsanits (INACTIVE)]

(In reply to Chris Peterson (:cpeterson) from comment #5)
> I bisected this bug (a second time) with a clean profile (so I am testing
> with the default value of dom.webcomponents.enabled) and found the same
> regression from bug 877072.

Thanks a lot for the work you put into this, I think I found the problems... indeed it was my patch :(

Comment 7

[Gabor Krizsanits (INACTIVE)]

Attachment: part1: import interface should be preffed

I tried to apply the pref for the whole partial interface but that did not work out, making the prop pref-ed worked out fine...

Comment 8

[Gabor Krizsanits (INACTIVE)]

Attachment: part2: import should not be a link type if pref is off

OK this line maybe a bit too long... I should have broken it, anyway the real question is if this code too hot or not for the pref check... NOTE: this patch is needed to make the chromium dashboard to work, meaning something relies on this link type check one way or another...

Comment 9

[Boris Zbarsky [:bzbarsky]]

> if this code too hot or not for the pref check

If this is at all a worry, use a pref cache.  It's about two lines of code.

Comment 10

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #9)
> > if this code too hot or not for the pref check
> 
> If this is at all a worry, use a pref cache.  It's about two lines of code.

You mean some addhoc bool member somewhere, or do we have some machinery for
pref cache? I was thinking about storing a bool flag just I have to change two
static functions signature and all their consumers to carry over the flag, that's why I
was hesitant about it...

Comment 11

[Boris Zbarsky [:bzbarsky]]

> or do we have some machinery for pref cache

See Preferences::AddBoolVarCache.

Comment 12

[Gabor Krizsanits (INACTIVE)]

Attachment: part2: import should not be a link type if pref is off v2

Comment 13

[Gabor Krizsanits (INACTIVE)]

Attachment: part2: import should not be a link type if pref is off. v3

whoops, left an unused bool mImportsEnabled; in the previous version...

Comment 15

[Gabor Krizsanits (INACTIVE)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6cf294e9e024
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a26ff9fadb0d

Comment 16

[Gabor Krizsanits (INACTIVE)]

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6cf294e9e024
https://hg.mozilla.org/mozilla-central/rev/a26ff9fadb0d

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 8434156 [details] [diff] [review]
part1: import interface should be preffed

Should imports not also be enabled for certified apps like document.registerElement() is?

Comment 19

[Gabor Krizsanits (INACTIVE)]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #18)
> Comment on attachment 8434156 [details] [diff] [review]
> part1: import interface should be preffed
> 
> Should imports not also be enabled for certified apps like
> document.registerElement() is?

Maybe it should... it's just not there yet I think. There are some critical issues to be fixed first like https://bugzilla.mozilla.org/show_bug.cgi?id=1016875#c7

Also, I'm not sure how much useful it would be without the nested import support...