1091002 - Crash @ xul!js::GetObjectClass+0x6

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Carsten Book [:Tomcat]]

Attachment: windgb data win 7 trunk debug build

Found via bughunter and reproduced on a win7 trunk debug build

steps to reproduce:
Load http://content.yudu.com/web/y5b2/0A2xfpx/2014WorldSeries/html/index.html#noRedirect 
--> The Build will crash but no visible assertion failure or so

Got this crash in windbg and could get a stack etc

Comment 1

[Carsten Book [:Tomcat]]

Attachment: bughunter stack

Comment 2

[Carsten Book [:Tomcat]]

Attachment: 2nd bughunter stack

Comment 3

[Boris Zbarsky [:bzbarsky]]

We're ending XMLHttpRequest::UpdateState with aUseCachedArrayBufferResponse true but aState.mResponse.isNull() also true.

Presumably a regression from bug 1008126...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, and I can reproduce on Mac, but not in my Linux debug build for some reason.  :(

Comment 5

[Lawrence Mandel [:lmandel] (use needinfo)]

If comment 3 is correct, Firefox 31 is unaffected.

Comment 6

[Shian-Yow Wu [:swu]]

(In reply to Boris Zbarsky [:bz] from comment #3)
> We're ending XMLHttpRequest::UpdateState with aUseCachedArrayBufferResponse
> true but aState.mResponse.isNull() also true.
> 
> Presumably a regression from bug 1008126...

Thanks for these information.  I can reproduce on my Linux with debug build(but not every time).

The issue occurs when mStateData.mResponse is JSVAL_NULL, and will crash when calling MOZ_ASSERT(JS_IsArrayBufferObject(mStateData.mResponse.toObjectOrNull()));

Looking at it.

Comment 7

[Shian-Yow Wu [:swu]]

When reproduced this issue, there was an abort event for the XHR request with array buffer type.  The abort event prevents the array buffer response from been transfered from main thread to worker thread XHR.  So, when this happens we should find a way to tell other event runners of same XHR not the use the cached array buffer response.

Comment 8

[Shian-Yow Wu [:swu]]

Attachment: Patch: Don't dispatch load/loadend events to worker XHR when been aborted.

When beened aborted, we should also drop load/loadend events to worker XHR.  Also, the original MOZ_ASSERT() is modified because JS_IsArrayBufferObject() doesn't accept null.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8514929 [details] [diff] [review]
Patch: Don't dispatch load/loadend events to worker XHR when been aborted.

khuey has looked at this code more recently than me, maybe he wants to stamp this? Otherwise kick it back to me and I'll try to relearn this.

Comment 10

[Shian-Yow Wu [:swu]]

Attachment: Patch: Drop loadend event from main thread to worker XHR if it's triggered by abort from worker.

Revised.

Comment 11

[Shian-Yow Wu [:swu]]

Clear the cache in "Advanced->Network->Cached Web Content" before loading the crash link, then we can easily reproduce the issue.

I made a flow diagram for the current worker XHR behavior in http://goo.gl/FL16Cx
The diagram helps to understand why we should drop the loadend event from main thread.

Comment 12

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

Comment on attachment 8517374 [details] [diff] [review]
Patch: Drop loadend event from main thread to worker XHR if it's triggered by abort from worker.

Review of attachment 8517374 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

That diagram is awesome btw.

Comment 13

[Shian-Yow Wu [:swu]]

Try server result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=602eddcf422a

https://hg.mozilla.org/integration/mozilla-inbound/rev/91b0cd0721a2

Comment 14

[Shian-Yow Wu [:swu]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #12)
> Comment on attachment 8517374 [details] [diff] [review]
> Patch: Drop loadend event from main thread to worker XHR if it's triggered
> by abort from worker.
> 
> Review of attachment 8517374 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me
> 
> That diagram is awesome btw.

Thank you.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/91b0cd0721a2

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

I spoke with khuey on irc. He doesn't think this fix needs to be uplifted and said the code is scary and should have more bake time. Given that we go to build with beta9 tomorrow, we're not going to fix this in 34.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

wontifixing for 35 as well, it can ride the trains.