653419 - WebGL crash [@ mozilla::WebGLContext::GetCanvasLayer(nsDisplayListBuilder*, mozilla::layers::CanvasLayer*, mozilla::layers::LayerManager*) ]

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:6.0a1) Gecko/20110427 Firefox/6.0a1
Build Identifier: 

Crashlogs:
https://crash-stats.mozilla.com/report/index/551ae92e-7c08-4a77-ad99-c3bc62110428
https://crash-stats.mozilla.com/report/index/d53c4e0b-232f-4202-bbb2-971e52110428
https://crash-stats.mozilla.com/report/index/ceabb40e-799a-4271-9aa9-110d52110428
https://crash-stats.mozilla.com/report/index/fc63c5ef-f2aa-466a-b4c1-cf5c82110428

Graphic info
Adapter Description - NVIDIA GeForce 8600 GT
Vendor ID - 10de
Device ID - 0402
Adapter RAM - 256
Adapter Drivers - nvd3dumx,nvwgf2umx,nvwgf2umx nvd3dum,nvwgf2um,nvwgf2um
Driver Version - 8.17.12.7061
Driver Date - 4-7-2011
Direct2D Enabled - true
DirectWrite Enabled - true (6.1.7601.17563)
WebGL Renderer - NVIDIA Corporation -- GeForce 8600 GT/PCI/SSE2 -- 3.3.0
GPU Accelerated Windows - 1/1 Direct3D 10

Reproducible: Always

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: handle null gl pointer

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: correctly mark WebGL context creation as successful

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: correctly mark WebGL context creation as successful

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: check for null gl ptr in SetDimensions NOP path

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

OK, I don't fully understand this bug, here's what I know.

There are at least 2 bugs here.

1) The first bug is the crash. It is a read at address 0 at this line of code:

void* native_surface = gl->GetNativeData(gl::GLContext::NativeImageSurface);

Since GetNativeData is a virtual method, that crash almost certainly means that the gl pointer is null. The 'handle null gl pointer' patch is a bruteforce fix for it, the 'check for null gl ptr in SetDimensions NOP path' patch is an attempt at a smart fix (see below).

2) The second bug is that the AppNotes entries for WebGL are unreliable because of a bug, which is fixed by the 'correctly mark WebGL context creation as successful' patch. So in any case I would like this patch to go it, it will at least allow us to get accurate crash reports.


Here's what I don't know:

The AppNotes say:
AdapterVendorID: 10de, AdapterDeviceID: 0402, AdapterDriverVersion: 8.17.12.7061
D2D? D2D+
DWrite? DWrite+
D3D10 Layers? D3D10 Layers+
WebGL? EGL? EGL-
WGL? GL Context? GL Context+
WGL+
WebGL+
WebGL-

There are 2 ways to interprete that.

1) one way is to say that the last WebGL- is produced by the above-discussed bug i.e. there never was a WebGL context creation failure, there just was a SetDimensions call that returned early here:
    if (mWidth == width && mHeight == height)
        return NS_OK;
The bug (null gl pointer) could the be explained in the scenario of a race condition between destroying and resizing a WebGL context.

2) another way is to take the WebGL- seriously, despite the above-discussed bug which can produce bogus WebGL-. One thing that pleads for this is the EGL-. Something is really wrong on this machine: despite using a recent NVIDIA driver, it failed to create a ANGLE EGL context. So it tried WGL as fallback. That worked once and, under this hypothesis, failed the second time. It is still not clear how we ended up calling GetCanvasLayer on a failed WebGL context object.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Ah! Didn't see that this is a 64bit build. That explains why EGL fails.

I suggest taking the 2nd and 3rd patch; perhaps we can pass on the 1st patch for now.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

@ Virtual_ManPL: 64bit builds on Windows are not supported yet.

Comment 8

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to comment #7)
> @ Virtual_ManPL: 64bit builds on Windows are not supported yet.

I know that "officially" 64bit builds of Firefox for Windows aren't supported yet, but someone need to test them and report bugs, so you guys will have less job to do in the future and we will have fewer bugs :)

Comment 9

[Raul Nicolae Malea]

Confirm this bug. Tested in Firefox 4 in default profile.

My reports:

https://crash-stats.mozilla.com/report/index/bp-05197bea-07b7-4e1e-a05e-eb0152110428

https://crash-stats.mozilla.com/report/index/1a3aac84-4f87-4c72-9a2f-ec2442110428

Mozilla/5.0 (Windows NT 6.1; rv:2.0) Gecko/20100101 Firefox/4.0 ID:20110318052756

Graphics:

Descrierea adaptorului NVIDIA GeForce 8600 GT 
ID furnizor 10de
ID dispozitiv 0402
RAM pentru adaptor 512
Drivere pentru adaptor  nvd3dum nvwgf2um,nvwgf2um
Versiune driver  8.17.12.6724
Dată driver 2-23-2011
Direct2D activat  true
DirectWrite activa  ttrue (6.1.7601.17563, font cache n/a)
Motor de afișare WebGL Google Inc. -- ANGLE -- OpenGL ES 2.0 (ANGLE 0.0.0.541)
Accelerare GPU Windows 1/1 Direct3D 10
      
      
      
        
        

My addons: 
Abduction! 3.0.10
About startup 0.1.5
about:me 0.5
about:response 0.1
Add-on Collector 2.0.1
Add-on Compatibility Reporter 0.8.3
AddonFox 1.1.7
Adobe Acrobat 10.0.1.434
Adobe Contribute Toolbar 6.0 [DISABLED]
Blog Comment Autofill 
British English Dictionary 1.19.1
Bugzilla Tweaks 1.10
ChatZilla 0.9.86.1
Converter 1.0.9
Crash Report Helper 1.2 [DISABLED]
Default 4.0 [DISABLED]
Dicționar românesc de corectare ortografică. 1.11
Download Statusbar 0.9.8
DownloadHelper 4.8.6
DownThemAll! 2.0.2
F1 by Mozilla Labs 0.8.3
FFixer 2.3.1
FileList V3 
fireblur 1260925626
Firefox 4 Menu Button with icon+(Tabs in Titlebar) 
Firefox B 1260925626 [DISABLED]
Flagfox 4.1.2
FlashGames Maximizer 3.1.3.86 [DISABLED]
FlashGot 1.2.9.3
FoxTab 1.4.2
German Dictionary 2.0.2
Google Earth Plugin 1.0.0.1
Google Shortcuts 2.1.6
Google Talk Plugin 1.9.2.0
Google Talk Plugin Video Accelerator 0.1.43.5
Google Update 1.3.21.53
Greasefire 1.0.4 [DISABLED]
Greasemonkey 0.9.2
Hard blockers counter 1.0.1 [DISABLED]
IE Tab 2 (FF 3.6+) 2.12.21.1
IE Tab Plug-in 2.2.0.1
ImTranslator 4.01
Java Deployment Toolkit 6.0.240.7 6.0.240.7
Java(TM) Platform SE 6 U24 6.0.240.7
Massive Extender 1.0b5
Microsoft Office 2010 14.0.4730.1010
Microsoft Office 2010 14.0.4761.1000
Minefield - Better Add-on Bar/Status Bar  [DISABLED]
Movie Torrent Search Linker (for IMDB, Rotten Tomatoes and Yahoo Movies) 0.3.7.9
Mozilla Labs: Prospector - Home Dash 10 [DISABLED]
Mozilla QA Companion 1.2.3
MozMill Crowd 0.1.2
Nightly and Aurora 1303328422 [DISABLED]
Nightly Tester Tools 3.1.6
NoScript 2.1.0.2
NVIDIA 3D Vision 7.17.12.6099
NVIDIA 3D VISION 7.17.12.6099
Personas 1.6.2
Picasa 3.0.0.0
Power Twitter 1.60
Prism for Firefox 1.0b4 [DISABLED]
Puppy Dogs... 1291695684 [DISABLED]
QuickTime Plug-in 7.6.9 7.6.9.0
Read It Later 2.1.1
RealJukebox NS Plugin 12.0.1.647
RealNetworks(tm) RealPlayer Chrome Background Extension Plug-In (32-bit)  12.0.1.647
RealPlayer Version Plugin 12.0.1.647
RealPlayer(tm) G2 LiveConnect-Enabled Plug-In (32-bit)  12.0.1.647
RealPlayer(tm) HTML5VideoShim Plug-In (32-bit)  12.0.1.647
ReminderFox 1.9.9.3.1
Sage 1.4.10
Shockwave Flash 10.2.159.1
Shockwave for Director 11.5.9.620
Show Just Image 2 2.5.3
Silverlight Plug-In 5.0.60401.0
startup.service 0.2
Status-4-Evar 2011.04.06.18
StumbleUpon 3.81
Stylish 1.1.2
Test Pilot 1.1
TortoiseSVN Menu 0.2.5
Ubiquity 0.6 [DISABLED]
URL Fixer 2.0.3
Userscripts Updater 
Windows Live Photo Gallery 15.4.3508.1109
WiseStamp 2.2.1
Yahoo Application State Plugin 1.0.0.7
Yoono 7.6.5
YouTube Enhancer 2010-12-18

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Raul: this seems like a separate bug, it would be a good idea to file it separately. Indeed your Firefox survives the line that's crashy in the original report here. That means that gl!=null in your case. The line that's crashy for you is
    HRESULT hr = mTexture->Map(0, D3D10_MAP_WRITE_DISCARD, 0, &map);
in CanvasLayerD3D10::Updated(). Could well be that the mTexture pointer was null.

Comment 11

[Raul Nicolae Malea]

Also confirmed in Nightly. Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110428 Firefox/6.0a1 ID:20110428030557

Crash reports:

https://crash-stats.mozilla.com/report/index/8257dd4e-5e84-4b40-8707-5053d2110428

https://crash-stats.mozilla.com/report/index/aaf756f6-7d42-48b8-8f48-8697e2110428

Comment 12

[Raul Nicolae Malea]

(In reply to comment #10)
> Raul: this seems like a separate bug, it would be a good idea to file it
> separately. Indeed your Firefox survives the line that's crashy in the original
> report here. That means that gl!=null in your case. The line that's crashy for
> you is
>     HRESULT hr = mTexture->Map(0, D3D10_MAP_WRITE_DISCARD, 0, &map);
> in CanvasLayerD3D10::Updated(). Could well be that the mTexture pointer was
> null.

Oh! Not same bug? Sorry.

Comment 13

[Raul Nicolae Malea]

And also, crash report for my Nighlty is different bug?

Comment 14

[novacrystallis]

I was the one that had problems originally on this and reported on MozillaZine. Here are my crash reports. Had earlier updated it to Catalyst 11.4 drivers.

https://crash-stats.mozilla.com/report/index/bp-41306687-9555-4e25-acfc-d77b72110427
https://crash-stats.mozilla.com/report/index/bp-6bcabeaf-8ada-48f7-9309-c1e222110428

Comment 15

[novacrystallis]

Sorry, here are the links.

https://crash-stats.mozilla.com/report/index/41306687-9555-4e25-acfc-d77b72110427
https://crash-stats.mozilla.com/report/index/6bcabeaf-8ada-48f7-9309-c1e222110428

Apparently, it started crashing in Firefox 4 also.

https://crash-stats.mozilla.com/report/index/3b612e41-4184-4951-bc8f-667a12110428
https://crash-stats.mozilla.com/report/index/490967be-0069-4729-bfbb-d6c0b2110428

Comment 16

[Johnathan Nightingale [:johnath]]

Definitely want to fix this, but Firefox 5 isn't supported on win64, so this doesn't need tracking-firefox5

Comment 17

[Joe Drew (not getting mail)]

Comment on attachment 528837 [details] [diff] [review]
handle null gl pointer

Review of attachment 528837 [details] [diff] [review]:

I'm not really in favour of adding null checks at seemingly arbitrary points in the code, and, as Benoit says in comment 6, we probably don't need this patch.

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/mozilla-central/rev/b23704f29948
http://hg.mozilla.org/mozilla-central/rev/844ada724179

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 528847 [details] [diff] [review]
check for null gl ptr in SetDimensions NOP path

These two patches are low-risk; one fixes a serious crasher (not just on Win64), the other one improves the quality of crash reports. Recommending for both Firefox 4 and Aurora/5.

Comment 20

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Verified on
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:6.0a1) Gecko/20110507 Firefox/6.0a1

But now I get other crash.
bug #655589

Comment 21

[Benoit Jacob [:bjacob] (mostly away)]

Replying on bug 655589: it seems that you're still not using a build of Firefox that has these fixes.

Comment 22

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Odd, it still happens in
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:6.0a1) Gecko/20110510 Firefox/6.0a1

https://crash-stats.mozilla.com/report/index/bp-e2f2976c-4de2-4960-a3d5-54a9d2110510

Comment 23

[Benoit Jacob [:bjacob] (mostly away)]

Can you please go to about:buildconfig and paste the "built from" link?

Comment 24

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Sure.

http://hg.mozilla.org/mozilla-central/rev/e0f6db50231f

Comment 25

[Benoit Jacob [:bjacob] (mostly away)]

Looks like a separate bug, though it could have caused the first bug that we fixed here.

You're crashing at:

if ((value = GetParsedAttr(nsGkAtoms::height)) &&
    value->Type() == nsAttrValue::eInteger)
{
    size.height = value->GetIntegerValue();  // <-- crash, deref null
} 

If that crash were caused by value being null, then you should have crashed above, at value->Type() already. Indeed, neither of the methods here is virtual.

So I rather think that GetIntegerValue() was inlined here (you're running an optimized build) and you're crashing inside of it:

inline PRInt32
nsAttrValue::GetIntegerValue() const
{
  NS_PRECONDITION(Type() == eInteger, "wrong type");
  return (BaseType() == eIntegerBase)
         ? GetIntInternal()
         : GetMiscContainer()->mInteger;
}

The crash could well be explained if for example GetMiscContainer() were returning null.

In any case please file this as a separate bug, against Canvas:2D, and please CC me.

Comment 26

[Benoit Jacob [:bjacob] (mostly away)]

Well, you can file as Canvas:WebGL if you want. I dont know what component is appropriate for generic HTMLCanvasElement code.

Comment 27

[JP Rosevear [:jpr]]

Please land for Aurora by Monday May 16 or the approval will potentially be lost.  Please mark as status-firefox5 fixed when you do.

Comment 28

[LegNeato]

Transplanted to mozilla-aurora:

    http://hg.mozilla.org/releases/mozilla-aurora/rev/dc3f31ef14f9
    http://hg.mozilla.org/releases/mozilla-aurora/rev/7f1a1f89a06a

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 528847 [details] [diff] [review]
check for null gl ptr in SetDimensions NOP path

Since we don't plan another 2.0-based release I assume you don't still want these approvals