Closed Bug 1091002 Opened 7 years ago Closed 7 years ago

Crash @ xul!js::GetObjectClass+0x6

Categories

(Core :: DOM: Workers, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox33 --- wontfix
firefox34 + wontfix
firefox35 + wontfix
firefox36 + fixed
firefox-esr31 --- unaffected

People

(Reporter: cbook, Assigned: swu)

References

()

Details

(Keywords: crash, regression)

Attachments

(4 files, 1 obsolete file)

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
Attached file bughunter stack
Attached file 2nd bughunter stack
We're ending XMLHttpRequest::UpdateState with aUseCachedArrayBufferResponse true but aState.mResponse.isNull() also true.

Presumably a regression from bug 1008126...
Component: JavaScript Engine → DOM: Workers
Flags: needinfo?(swu)
Flags: needinfo?(bent.mozilla)
Oh, and I can reproduce on Mac, but not in my Linux debug build for some reason.  :(
Blocks: 1008126
Keywords: regression
(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.
Assignee: nobody → swu
Flags: needinfo?(swu)
Flags: needinfo?(bent.mozilla)
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.
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.
Attachment #8514929 - Flags: review?(bent.mozilla)
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.
Attachment #8514929 - Flags: review?(bent.mozilla) → review?(khuey)
Revised.
Attachment #8514929 - Attachment is obsolete: true
Attachment #8514929 - Flags: review?(khuey)
Attachment #8517374 - Flags: review?(khuey)
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 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.
Attachment #8517374 - Flags: review?(khuey) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
https://hg.mozilla.org/mozilla-central/rev/91b0cd0721a2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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.
wontifixing for 35 as well, it can ride the trains.
You need to log in before you can comment on or make changes to this bug.