Closed Bug 1019319 Opened 6 years ago Closed 6 years ago

Firefox can no longer load Chromium Dashboard after HTML Imports landed

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox31 --- unaffected
firefox32 + fixed

People

(Reporter: cpeterson, Assigned: gkrizsanits)

References

()

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

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
Assignee: nobody → gkrizsanits
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?
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.
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(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.
(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
(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 :(
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...
Attachment #8434156 - Flags: review?(mrbkap)
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...
Attachment #8434158 - Flags: review?(mrbkap)
> 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.
(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...
> or do we have some machinery for pref cache

See Preferences::AddBoolVarCache.
Attachment #8434158 - Attachment is obsolete: true
Attachment #8434158 - Flags: review?(mrbkap)
Attachment #8434241 - Flags: review?(mrbkap)
whoops, left an unused bool mImportsEnabled; in the previous version...
Attachment #8434241 - Attachment is obsolete: true
Attachment #8434241 - Flags: review?(mrbkap)
Attachment #8434243 - Flags: review?(mrbkap)
Duplicate of this bug: 1020240
Attachment #8434156 - Flags: review?(mrbkap) → review+
Attachment #8434243 - Flags: review?(mrbkap) → review+
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?
(In reply to Johnny Stenback (:jst, jst@mozilla.com) 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...
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.