Closed Bug 1033972 Opened 7 years ago Closed 7 years ago

BackgroundPageThumbs.captureIfMissing() and PageThumbs.captureAndStoreIfStale() both incorrectly use PromiseWorker

Categories

(Toolkit :: General, defect)

x86_64
Windows 8.1
defect
Not set
normal
Points:
5

Tracking

()

RESOLVED FIXED
mozilla33
Iteration:
33.2

People

(Reporter: Unfocused, Assigned: Unfocused)

References

Details

Attachments

(1 file)

Both BackgroundPageThumbs.captureIfMissing() and PageThumbs.captureAndStoreIfStale() use APIs that return promises via PromiseWorker. However, they're using the resolved value as if it's wrapped in an object - which is wrong. ie, they're using the resolved value as:
  {ok: returned_value}
Instead of:
  returned_value


This code started using PromiseWorker about a year ago - before that, the resolved value was wrapped in an object. So the bug has been around that long - using an invalid return value. Which is worrisome, since the tests have been passing. And, in fact, still pass with the bug corrected. This would mean that we've been re-capturing thumbnails when we shouldn't be.
Marco, could you add this to the current iteration? It's blocking me in bug 994915.
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Points: --- → 5
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Aha! So, browser_thumbnails_bg_captureIfMissing.js is passing currently because it compares the timestamp of the file. But, if the file already exists, we don't overwrite it anyway. So it's correctly checking that the file isn't overwritten, but it's not checking if the thumbnail is actually recaptured or not (and just not stored).

And browser_thumbnails_capture.js passes because simpleCaptureTest calls captureAndStoreIfStale() but doesn't wait for it to finish.
Attached patch Patch v1Splinter Review
This will result in some bandwidth savings for some people...
Attachment #8450090 - Flags: review?(adw)
Added to Iteration 33.2
Iteration: --- → 33.2
QA Whiteboard: [qa?]
Flags: needinfo?(mmucci)
Wow...

... is what I thought at first, but are you sure?  My tree is a few days out of date, but this bug definitely doesn't exist on it -- I just made sure.  Bug 801598 landed yesterday and changed the PageThumbs worker implementation.  I'll rebuild and see if I can reproduce, but I'm betting not, because it sure looks like messages are still wrapped like { ok: data }:

dispatch() called and result is wrapped here and passed to postMessage:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/promiseworker/worker/PromiseWorker.js#122

onmessage receives the message here, looks for `"ok" in data`, and passes `data` along unmodified:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/promiseworker/PromiseWorker.jsm#217
Oh, but BasePromiseWorker.post does return/yield reply.ok: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/promiseworker/PromiseWorker.jsm#255

So maybe this is due to bug 801598.
Yeah, I can reproduce after updating my tree.  I haven't done a bisect or anything, but just reading the code and given my previous comment, I'm going to blame bug 801598. :-)  Blair, can you confirm that you saw this bug on a tree that included bug 801598?
Blocks: 801598
Attachment #8450090 - Flags: review?(adw) → review+
Today I learned that in JS boolean.foo is legal (and undefined).
Oh, yes, this was due to bug 801598. For some reason I read that bug as having landed ages ago - sorry for the alarm bells in your head :\
QA Whiteboard: [qa?] → [qa-]
https://hg.mozilla.org/mozilla-central/rev/54cb0b2e118b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.