1244480 - Firefox 44.0 doesn't execute livecode emscripten code any more

Status: VERIFIED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Hermann]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20160123151951

Steps to reproduce:

Tried to load with the new Firefox 44.0
(and also tested with the FirefoxEdition 45.0a2 (2016-01-25))
my HTML5 emscripten generated standalones
http://hyperhh.org/html5/index-large.html


Actual results:

Loading seems to be the same as before version 44.0.
But after laoding (from server or cache) the HTML5-canvas is empty (doesn't display).

The result is the same for different OS (MacOS X, Win 10, Linux Ubuntu).
The result is the same when using hardware acceleration or not.
The result is the same when using multiprocesses support or not.


Expected results:

The versions before 44.0 Firefox were by far the best with caching and the best running browser for emscripten code generated by LiveCode's HTML5 standalone engine.
When loading from server I had < 7 seconds, from cache < 2 seconds, with Firefox only, others (Chrome, Opera, Safari) were clearly slower.
The same when running the code: Firefox before 44.0 was up to 5 times faster than the others.

Comment 1

[Tooru Fujisawa [:arai]]

Tested with this page: http://hyperhh.org/html5/SundayGameNb2-8.0.0-dp-9X.html

After loading, following error is shown in console.

> TypeError: Value can't be converted to a dictionary.
>   standalone-commercial-8.0.0-dp-9.js:1:292806

The actual code is following:
  canvas.getContext("2d",false)

The 2nd argument is not an object, and it causes the exception.


Regression range:
  https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e8c7dfe727cd970e2c3294934e2927b14143c205&tochange=b5e0aa4ec5117a4ddcb095d2fd39bcc07b9b9d4d

Backing out revision b5e0aa4ec511 [1] (bug 1215072) fixes the issue on latest m-c, but the same error is still reported to console, as it's mentioned in the bug.


HTML spec [2] says that we should propagate exception while converting the passed argument to dictionary:
> 3. Let dict be the result of converting jsval to the dictionary type
>    CanvasRenderingContext2DSettings. If this throws an exception, then
>    propagate the exception and abort these steps.

and WebIDL spec [3] says converting a boolean value to dictionary should throw an exception:
> 1. If Type(V) is not Undefined, Null or Object, then throw a TypeError.

So, this looks like a spec issue about web compat, either in HTML or WebIDL (maybe HTML?).
Will file a spec bug.


Anyway, we might be better fixing the implementation at least to align with other browsers, ahead of the spec fix, to address the regression.

[1] https://hg.mozilla.org/mozilla-central/rev/b5e0aa4ec511
[2] https://html.spec.whatwg.org/multipage/scripting.html#coerce-context-arguments-for-2d
[3] https://heycam.github.io/webidl/#es-dictionary

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: minimal testcase

Here's the testcase that calls |canvas.getContext("2d",false)|.

Safari, Chrome, and Edge don't throw any exception.

Comment 3

[Tooru Fujisawa [:arai]]

filed spec bug: https://github.com/whatwg/html/issues/595

Comment 4

[Tooru Fujisawa [:arai]]

[Tracking Requested - why for this release]:

Regression happens from Firefox 44 (bug 1215072), that is caused by making HTMLCanvasElement.getContext to follow the spec (bug 645792 + bug 1215072), that does not match to other browsers and previous Firefox releases.
it breaks websites that rely on previous behavior.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

I would prefer if the site was fixed, given that we're just being consistent with webidl handling and this case indicates there is an error in the page's code.

But if fixing the site isn't possible, need to add some special case to the spec.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Has anyone reported to the website about the issue in its code?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Tooru Fujisawa [:arai] from comment #4)
> it breaks websites that rely on previous behavior.
websites or a website? Do we know if this has happened more often?

Comment 8

[Tooru Fujisawa [:arai]]

(In reply to Olli Pettay [:smaug] from comment #7)
> websites or a website? Do we know if this has happened more often?

sorry, "a website" at this point.


Hermann, where does the code related to canvas come from?  is it also a part of LiveCode?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Looks like Chrome does throw exception in case one does
canvas.getContext("2d", { get alpha() { throw "FooBarError"; } });
But not when just {}, or "" or 123 or false is passed as the 2nd param.
Obviously passing {} works also in Firefox since that is what the spec allow (alpha defaults to true)


Looks like chromiums code here is really odd.
https://codereview.chromium.org/795833004 made them to use the same dictionary for all the context types.
So document.body.innerHTML = "<canvas>"; document.body.firstChild.getContext("2d", { get stencil() { throw "Foobar"; } }); throws in Chrome, even though it should not per spec.

Comment 10

[Tooru Fujisawa [:arai]]

Chrome ignores non-object value, in following code.

https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blink/bindings/core/v8/V8CanvasContextCreationAttributes.cpp&q=CanvasContextCreationAttributes&sq=package:chromium&dr=CSs&l=20
> void V8CanvasContextCreationAttributes::toImpl(v8::Isolate* isolate,
> v8::Local<v8::Value> v8Value, CanvasContextCreationAttributes& impl,
> ExceptionState& exceptionState)
> {
>     if (isUndefinedOrNull(v8Value)) {
>         return;
>     }
>     if (!v8Value->IsObject()) {
>         // Do nothing.
>         return;
>     }


WebKit seems not to support 2nd argument on "2d" type.
on webgl, it converts boolean to Boolean object

https://github.com/WebKit/webkit/blob/6bc6a03d1caa8702af53d1a46114e0977ce622b9/Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp#L101
> static void get3DContextAttributes(ExecState& state, RefPtr<CanvasContextAttributes>& attrs)
> {
>     JSValue initializerValue = state.argument(1);
>     if (initializerValue.isUndefinedOrNull())
>         return;
>     
>    JSObject* initializerObject = initializerValue.toObject(&state);
> ...
> ...
> JSValue JSHTMLCanvasElement::getContext(ExecState& state)
> {
>     HTMLCanvasElement& canvas = wrapped();
>     const String& contextId = state.argument(0).toString(&state)->value(&state);
>     
>     RefPtr<CanvasContextAttributes> attrs;
> #if ENABLE(WEBGL)
>     if (HTMLCanvasElement::is3dType(contextId)) {
>         get3DContextAttributes(state, attrs);
>         if (state.hadException())
>             return jsUndefined();
>     }
> #endif

Comment 11

[Tooru Fujisawa [:arai]]

The official demo on LiveCode page causes same error.
  http://livecode.com/products/livecode-platform/html5/demos/calculator/
> TypeError: Value can't be converted to a dictionary.
>   standalone-community.js:1:194375

The code is also same:
> canvas.getContext("2d",false)

I guess the error in the comment #0 is the same thing.


They says that HTML5 deployment is supported from Developer Preview (DP4) that was released 2015-08-31, and it's marked as experimental.
and there seems to be no newer versions (am I correct?)

http://downloads.livecode.com/livecode/8_0_0/LiveCodeNotes-8_0_0_dp_4.pdf
> Support for the web platform using HTML5 (8.0.0-dp-4 - experimental)
> 
> The LiveCode engine will now run in web browsers that support HTML5.
> This means that you can now deploy simple LiveCode apps to users without any
> installation required.
> To deploy a stack as an HTML5 application, enable the "Build for HTML5"
> checkbox on the "HTML5" page of the standalone settings window, and then
> generate the standalone in the normal way.
> For more information on HTML5 deployment, including options for embedding
> LiveCode standalones i web pages, please see the "HTML5 Deployment" guide in
> the IDE dictionary.
> 
> Important: This feature is currently experimental. This means that it may not
> be complete or may fail in some circumstances that you would expect it to
> work. Please do not be afraid to try it out as we need feedback to develop it
> further.

so, at least for this case, it should be better contacting them and wait for the fix on their side, instead of reverting the behavior on Firefox.

Comment 12

[Hermann]

Hi all,

sorry for the late answer. I am an (advanced) user, not member of the LC-developer team,
and can't see at the moment what could remove the issue in the emscripten calling code.
The js exception is with ALL latest version of the LiveCode HTML5 standalone engine
(via emscripten).

This doesn't depend in any way on my special website (an alternate server with
the same content is http://hh.on-rev.com/html5/).

The Livecode-emscripten engine is open source at github
( https://github.com/livecode/livecode ).
The current LiveCode version is 8.0.0-dp13. There were bugs in dp10-dp12
(relating to speed only) that caused me to still use dp9.

Safari, Chrome and Opera work with all standalones on my site. My examples are
free of errors. One complicated example, good for testing (with respect to the canvas)
is http://hyperhh.de/html5/asianRodsHTML-8.0.0-dp-9X.html.

Microsoft products (IExplorer, Edge) didn't work at all. They load, but don't display.

Firefox before 44.0 (and even the 'derivate' Iceweasel on RaspberryPi) loaded fastest,
displayed fast. [Could't only avoid the blinking cursor at topleft of the canvas].

I informed the leader of the LiveCode developer team, he's on congress until monday
morning. But he will come in to answer your special questions.

I hope so much that this js-exception can be removed -- I'll get soon a review of the
"math-related clocks" on my site (lower half of the examples) in the american
math-online-magazin "Math Munch". I told them to tell their readers to use Firefox
because other browser are not fast enough with the emscripten output ...

Thank you very much for your interest and help.

Kind regards from Germany, Hermann

Comment 13

[Tooru Fujisawa [:arai]]

Thank you for the detail, and contacting them :)

The actual code seems to be in following file:

https://github.com/livecode/livecode/blob/8db7eda7f746ec7babf2e06debedf4ef746c05d3/engine/src/em-surface.js#L29
>         // Create and get a 2D rendering context for the canvas
>         var context = canvas.getContext('2d', false);
>         if (!context) {
>             context = Browser.createContext(canvas, false, true);
>         }

the code sounds like some compatibility testing thing.
anyway, if the "false" there has no meaning for other platform, just removing it will fix the issue.


Also, similar case is found in web-platform-test, and looks like have been imported to WebKit and chromium.

https://github.com/w3c/web-platform-tests/blob/master/html/semantics/embedded-content/the-canvas-element/2d.getcontext.extraargs.html#L22
https://github.com/WebKit/webkit/blob/master/LayoutTests/imported/w3c/canvas/2d.getcontext.extraargs.html#L24
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/LayoutTests/canvas/philip/tests/2d.getcontext.extraargs.html&l=15
> <p class="desc">The 2D context ignores extra getContext arguments</p>
> ...
> _assertDifferent(canvas.getContext('2d', false, {}, [], 1, "2"), null, "canvas.getContext('2d', false, {}, [], 1, \"2\")", "null");

at least the test does not match to the spec.

Comment 14

[peter]

I'm the upstream LiveCode developer responsible for our Emscripten/JS port.  Tracking bug: http://quality.livecode.com/show_bug.cgi?id=16795

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Since we're trying to figure out what to do with this 
(change the HTML spec and make Gecko to follow the updated spec, or let the web site to update its code), would help to understand why 'false' is passed as the 2nd parameter.
Peter, was that code copied from elsewhere or just some accidental mistake or what?

Comment 16

[peter]

Unfortunately, I didn't write that piece of code, and the engineer who did is on a two-week vacation.  I suspect he just copied the code from an example of how to use the Canvas API.  I have no particular objection to changing it.

I'm happy to change it to (insert anything that will work in Firefox, Chrome and Safari).  What's recommended?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

false as the second argument doesn't mean anything, so removing it would be the right thing to do.
Whether or not spec needs changes here is being discussed in https://github.com/whatwg/html/issues/595

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

But I think I'll patch this still in Gecko too, for now.

Comment 19

[Hermann]

Thank you all for your investigations and help.
Also I learned a lot from your discussions.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: bring back buggy behavior

I'm not happy with this, but perhaps we should do it after all.
So, let random stuff to be passed as argument, but only try to interpret it as a dictionary when it is 
an object.

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

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Attachment: without extra newline

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8715953 [details] [diff] [review]
without extra newline

r=me pending the spec getting sorted out.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Attachment: null value, not undefined.

Looks like bindings layer has, what I'd call a bug, an assertion where it asserts dictionary init takes null value, yet it later deals with null or undefined.
See
https://treeherder.mozilla.org/logviewer.html#?job_id=16351265&repo=try

So this just uses JS::NullHandleValue and not JS::UndefinedHandleValue
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc28eeae44f8





Approval Request Comment
[Feature/regressing bug #]: bug 1215072
[User impact if declined]: site(s) which used to work, don't anymore
[Describe test coverage new/current, TreeHerder]: The patch is adding tests
[Risks and why]: Should be low risk to bring back the old behavior (well, old behavior without the bug in it where exception wasn't handled properly)
[String/UUID change made/needed]: NA

So I consider this as a low risk enough even for mozilla-release, if we do a . release

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8716020 [details] [diff] [review]
null value, not undefined.

Unless the issue is severe enough (only one site is impacted so far) and the fix has been validated, I would hesitate to take this as a ride-along in 44.0.1 release. Sorry!

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8716020 [details] [diff] [review]
null value, not undefined.

Fixes a recent regression, adds test coverage. OK to uplift to aurora.

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53a2e89326e5

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/53a2e89326e5

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8716020 [details] [diff] [review]
null value, not undefined.

Fix a regression, taking it.
Should be in 45 beta 4

Comment 29

[Kohei Yoshino [:kohei]]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/htmlcanvaselement-getcontext-no-longer-accepts-non-object-context-attributes/

Comment 30

[Masatoshi Kimura [:emk]]

(In reply to Kohei Yoshino [:kohei] from comment #29)
> Posted the site compatibility doc:
> https://www.fxsitecompat.com/en-CA/docs/2016/htmlcanvaselement-getcontext-no-
> longer-accepts-non-object-context-attributes/

Why en-CA instead of en-US, by the way?

Comment 31

[Kohei Yoshino [:kohei]]

(In reply to Masatoshi Kimura [:emk] from comment #30)
> Why en-CA instead of en-US, by the way?

Because I live in Toronto and many subscribers are in Europe, so that en-CA/GB spellings are better for me and them :)

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dbf841cfa0df

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ca3a698f224b

Comment 34

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

While this is a valid issue, it does not sound so critical to warrant (inclusion in) a dot release. Wontfix for Fx44.

Comment 35

[Hermann]

Thank you all again for your efforts and help, I'm impressed.

I'm 66 and have 5 children, but I'm still learning every day,
so please allow me this last question Ritu:

What are criterions for "sounding critical"?

Kind regards, Hermann

Comment 36

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Hermann from comment #35)
> Thank you all again for your efforts and help, I'm impressed.
> 
> I'm 66 and have 5 children, but I'm still learning every day,
> so please allow me this last question Ritu:
> 
> What are criterions for "sounding critical"?
> 
> Kind regards, Hermann

Hermann, thank you for your patience. A critical issue is something that usually impacts a significant # of end-users (think hundred thousand or millions). We used that as a data point in deciding whether to do a dot release for this issue or not.

Having said that, we now have a fix for this in Beta45. :) Could you please verify this issue is fixed as expected in a latest 45/46/47 build? Many Thanks!

Comment 37

[Hermann]

Ritu,

... accepted, of course.

I read the changed source for 45/46/47 and tried just now again "live"
FirefoxDeveloperEdition 46.0a2 (2016-03-01) from update channel aurora.
It works here fine again.

Firefox is in average 3 times faster than Safari, Chrome and Opera!!
Although the others now also improved their local chaching.

There was also a fix from LiveCode's side (essentially based on Tooru's
and Olli's hint), small but very effective team there.
So my projects run, recompiled, also with Firefox 44 again, all other's
have also to recompile or wait until visitors have Firefox 45/46/47 ...

Thank you all again. I think I got a taste of 'Why Firefox is the leading
browser'.

Hermann

Comment 38

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Hermann from comment #37)
> Ritu,
> 
> ... accepted, of course.
> 
> I read the changed source for 45/46/47 and tried just now again "live"
> FirefoxDeveloperEdition 46.0a2 (2016-03-01) from update channel aurora.
> It works here fine again.
> 
> Firefox is in average 3 times faster than Safari, Chrome and Opera!!
> Although the others now also improved their local chaching.
> 
> There was also a fix from LiveCode's side (essentially based on Tooru's
> and Olli's hint), small but very effective team there.
> So my projects run, recompiled, also with Firefox 44 again, all other's
> have also to recompile or wait until visitors have Firefox 45/46/47 ...
> 
> Thank you all again. I think I got a taste of 'Why Firefox is the leading
> browser'.
> 
> Hermann

Amazing! Thank you for a super prompt follow up.