Last Comment Bug 802395 - frameworker json.parse occasional errors
: frameworker json.parse occasional errors
Status: VERIFIED FIXED
[Fx17]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 19
Assigned To: Shane Caraveo (:mixedpuppy)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-16 15:32 PDT by Shane Caraveo (:mixedpuppy)
Modified: 2012-12-04 15:16 PST (History)
6 users (show)
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified


Attachments
parsefix.patch (981 bytes, patch)
2012-10-16 15:32 PDT, Shane Caraveo (:mixedpuppy)
markh: feedback+
Details | Diff | Splinter Review
parsefix.patch (4.48 KB, patch)
2012-10-16 16:50 PDT, Shane Caraveo (:mixedpuppy)
gavin.sharp: review+
gavin.sharp: approval‑mozilla‑aurora+
gavin.sharp: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Shane Caraveo (:mixedpuppy) 2012-10-16 15:32:05 PDT
Created attachment 672057 [details] [diff] [review]
parsefix.patch

I've been seeing occasional failures with json.parse in the frameworker ClientPort class, and couldn't figure out a repro for it.  Then with reload, Pam started running into it and had to work around it.  I finally chased it down to pending messages on the ports.  

fw_AbstractPort_onmessage first parses the json it receives, then if there is no handler it sticks that object in pending messages

When the port connects, it grabs the pending messages and calls fw_AbstractPort_onmessage again, but passing an object rather than a string.

with reload, content area's are more likely to create pending messages while the frameworker is reloading, thus easier to reproduce, and more likely to be a problem.

The attachement looks like the correct fix to me, but the port stuff is pretty involved, will ask Mark to look.
Comment 1 Mark Hammond [:markh] 2012-10-16 15:42:38 PDT
Comment on attachment 672057 [details] [diff] [review]
parsefix.patch

I agree that looks correct but think we need a test :)

So IIUC, to see this you will need to have a port which is connected but which has not yet set an onmessage handler when a message arrives?  Our tests probably don't do that, but it shouldn't be too hard to arrange in browser_frameworker?
Comment 2 Shane Caraveo (:mixedpuppy) 2012-10-16 16:50:33 PDT
Created attachment 672100 [details] [diff] [review]
parsefix.patch

https://tbpl.mozilla.org/?tree=Try&rev=3e86045e2829
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-16 16:57:19 PDT
Comment on attachment 672100 [details] [diff] [review]
parsefix.patch

good catch
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-16 16:57:49 PDT
Comment on attachment 672100 [details] [diff] [review]
parsefix.patch

[Triage Comment]
a+ for 17/18 given that it's a social-specific, simple fix
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-17 18:44:34 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/77deb50b07e1
Comment 6 Ed Morley [:emorley] 2012-10-18 10:35:31 PDT
https://hg.mozilla.org/mozilla-central/rev/77deb50b07e1
Comment 7 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-21 09:29:07 PDT
Please land to branches by EOD Pacific time on Monday so we're sure to get this into 17.0 Beta 3
Comment 8 Scoobidiver (away) 2012-10-25 06:00:37 PDT
Beta 3 and Aurora missed despite the approval!
Comment 9 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-25 11:42:43 PDT
(In reply to Scoobidiver from comment #8)
> Beta 3 and Aurora missed despite the approval!

It's ok - I checked with gavin in irc ahead of time, we can take this in Beta 4 - please land this asap though, since it does have approval.
Comment 11 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-12-04 15:16:10 PST
Marking verified fixed since this landed with tests. Please add verifyme keyword if there's something you need QA to test.

Note You need to log in before you can comment on or make changes to this bug.