914920 - about:newtab displays thumbnail from unrelated websites for some links

Status: VERIFIED FIXED

(Toolkit :: General, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: Screenshot.png

On two different MBPs running Nightly 26, one of the nine links on my about:newtab page has a thumbnail from an unrelated website.

Please see the attached screenshot of an sfgate.com thumbnail linking to TBPL2. My about:newtab page also has an sfgate.com thumbnail that links to the correct website. Perhaps this bug is a thumbnail caching problem?

Comment 1

[Chris Peterson [:cpeterson]]

Clicking on the link with the incorrect thumbnail will refreshes about:newtab's thumbnail, fixing the problem.

Comment 2

[Drew Willcoxon :adw]

Thanks for filing.  Would you happen to have steps to reproduce?

Comment 3

[Chris Peterson [:cpeterson]]

(In reply to Drew Willcoxon :adw from comment #2)
> Thanks for filing.  Would you happen to have steps to reproduce?

Sorry, no STR. The bug is reproducible when loading the same profile in different Firefox versions (23-26), so the problem seems to be related to saving the thumbnails in my profile, not reading them.

Is there any information from my profile that would help you debug the problem? I can give you a copy of my profile, but I don't want attach it to a public bug. <:)

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Are both of these thumbnails pointing to the same background-image? You can use the inspector to find out. What are the image URLs? What are the link URLs?

Comment 5

[Chris Peterson [:cpeterson]]

My newtab page now has two mismatched thumbnails and links. The problem is the saved thumbnails. If I load "moz-page-thumb://thumbnail?url=https%3A%2F%2Fbugzilla.mozilla.org%2F" in the address bar, the thumbnail is the incorrect sfgate.com image, not bugzilla.mozilla.org.

Using the inspector, I see that background-images are correct in the HTML tag, but do not match the thumbnail image that is displayed. For example, here is the HTML for a bugzilla.mozilla.org link with a bugzilla.mozilla.org background-image that shows up as the sfgate.com home page:

<a class="newtab-link" title="Bugzilla Main Page https://bugzilla.mozilla.org/" href="https://bugzilla.mozilla.org/">
    <span class="newtab-thumbnail" style="background-image: url("moz-page-thumb://thumbnail?url=https%3A%2F%2Fbugzilla.mozilla.org%2F");"></span>
    <span class="newtab-title">
        Bugzilla Main Page
    </span>
</a>

Comment 6

[Drew Willcoxon :adw]

Luckily I ran across this and can reliably but inconsistently reproduce.  What I'm seeing is that some captures are queued up, the first finishes, and then the second finishes.  The problem is that this._webNav.currentURI is still the first URL when the second finishes.  So we capture the first page again and store it under the second URL.  The fact that the image stored under the second URL is actually the first page shows that the first page really is still loaded in the browser.

I don't understand what's going on.  The second page finishes with a status of NS_BINDING_ABORTED, which the idl says means the request was canceled [1].  OK, maybe we should account for canceled requests?  We currently don't.  But who canceled it?  And why does it cause the next queued capture to time out?

I can't prove it yet, but I think it may have something to do with loading about:blank after each capture.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIRequest.idl#48

Comment 7

[Drew Willcoxon :adw]

... All of that is in the background service.

Comment 8

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Drew Willcoxon :adw from comment #6)
> Luckily I ran across this and can reliably but inconsistently reproduce.

Cool!  What kinds of requests does it fail for?  And I guess this also means it's easy to test your theory re about:blank?

> [1].  OK, maybe we should account for canceled requests?  We currently
> don't.  But who canceled it?  And why does it cause the next queued capture
> to time out?

The way I read your description, it sounds like the second one is aborted so early that currentURI fails to be reset?  Or maybe it is the load of about:blank at the end of the first one being aborted?

Anyway, one scenario where I *can* see this happening somewhat legitimately is if we attempt to capture an invalid image (ie, the URL is of the image itself, and it's corrupt - eg [1]).  It seems this will abort the channel without giving us anything to capture.  There are probably other similar cases too - so yeah, I think we should handle aborted channels in some sane way.  Assuming the URLs we fail for aren't actually invalid though, it sounds like this will not completely solve the problem - we will just end up not capturing site 2 instead of giving it the wrong thumbnail - an improvement, but still not ideal :)

[1] http://mxr.mozilla.org/mozilla-central/source/image/src/imgRequest.cpp#830

Comment 9

[Drew Willcoxon :adw]

OK, what I'm seeing is that the about:blank we load after each page load sometimes ends up starting after we call load() on the next capture.  This about:blank load stops the next capture's load very early on, even before the docshell's current URI has been updated to the next capture's URI.

So the sequence of events where the bug happens is:

1. onStateChange uri1 STOP
2. load(about:blank)
3. onStateChange uri2 START
4. onStateChange uri2 STOP
5. onStateChange about:blank START

This happens even when load(about:blank) is replaced with docShell.createAboutBlankContentViewer(null), as Mark suggested trying.

So the about:blank START happens after the uri2 STOP, but where's the proof that the former causes the latter?  Here are the failing and successful stacks at the time uri2 STOP is called.  Note that I'm using docShell.createAboutBlankContentViewer(null) instead of load(about:blank).

Stack where bug happens:

> ... xpc_DebuggerKeywordHandler frames...
> #10 0x000000010341fed8 in nsDocLoader::DoFireOnStateChange (this=0x11164b400, aProgress=0x11164b428, aRequest=0x11a4d6c88, aStateFlags=@0x7fff5fbf710c, aStatus=NS_BINDING_ABORTED) at /Users/adw/fx/uriloader/base/nsDocLoader.cpp:1333
> #11 0x000000010341f962 in nsDocLoader::doStopDocumentLoad (this=0x11164b400, request=0x11a4d6c88, aStatus=NS_BINDING_ABORTED) at /Users/adw/fx/uriloader/base/nsDocLoader.cpp:878
> #12 0x000000010341e45b in nsDocLoader::DocLoaderIsEmpty (this=0x11164b400, aFlushLayout=true) at /Users/adw/fx/uriloader/base/nsDocLoader.cpp:755
> #13 0x000000010341f2f3 in nsDocLoader::OnStopRequest (this=0x11164b400, aRequest=0x11a4d6c88, aCtxt=0x0, aStatus=NS_BINDING_ABORTED) at /Users/adw/fx/uriloader/base/nsDocLoader.cpp:639
> #14 0x000000010341f6dd in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) (this=0x11164b408, aRequest=0x11a4d6c88, aCtxt=0x0, aStatus=NS_BINDING_ABORTED) at /Users/adw/fx/uriloader/base/nsDocLoader.cpp:643
> #15 0x000000010027a5a2 in nsLoadGroup::RemoveRequest (this=0x10f026020, request=0x11a4d6c88, ctxt=0x0, aStatus=NS_BINDING_ABORTED) at /Users/adw/fx/netwerk/base/src/nsLoadGroup.cpp:685
> #16 0x0000000100278c7b in nsLoadGroup::Cancel (this=0x10f026020, status=NS_BINDING_ABORTED) at /Users/adw/fx/netwerk/base/src/nsLoadGroup.cpp:287
> #17 0x000000010341dea3 in nsDocLoader::Stop (this=0x11164b400) at /Users/adw/fx/uriloader/base/nsDocLoader.cpp:268
> #18 0x0000000103411e35 in nsDocShell::Stop (this=0x11164b400) at nsDocShell.h:177
> #19 0x00000001033f1c43 in nsDocShell::CreateAboutBlankContentViewer (this=0x11164b400, aPrincipal=0x0, aBaseURI=0x0, aTryToSaveOldPresentation=true) at /Users/adw/fx/docshell/base/nsDocShell.cpp:7110
> #20 0x00000001033f24fc in nsDocShell::CreateAboutBlankContentViewer (this=0x11164b400, aPrincipal=0x0) at /Users/adw/fx/docshell/base/nsDocShell.cpp:7176
> docShell.createAboutBlankContentViewer(null) JS frames, ...

Stack where bug does not happen:

> ... xpc_DebuggerKeywordHandler frames...
> #10 0x000000010341fed8 in nsDocLoader::DoFireOnStateChange (this=0x11164f400, aProgress=0x11164f428, aRequest=0x119b53c88, aStateFlags=@0x7fff5fbfbe8c, aStatus=NS_OK) at /Users/adw/fx/uriloader/base/nsDocLoader.cpp:1333
> #11 0x000000010341f962 in nsDocLoader::doStopDocumentLoad (this=0x11164f400, request=0x119b53c88, aStatus=NS_OK) at /Users/adw/fx/uriloader/base/nsDocLoader.cpp:878
> #12 0x000000010341e45b in nsDocLoader::DocLoaderIsEmpty (this=0x11164f400, aFlushLayout=true) at /Users/adw/fx/uriloader/base/nsDocLoader.cpp:755
> #13 0x000000010341f2f3 in nsDocLoader::OnStopRequest (this=0x11164f400, aRequest=0x119b7a260, aCtxt=0x0, aStatus=NS_OK) at /Users/adw/fx/uriloader/base/nsDocLoader.cpp:639
> #14 0x000000010341f6dd in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) (this=0x11164f408, aRequest=0x119b7a260, aCtxt=0x0, aStatus=NS_OK) at /Users/adw/fx/uriloader/base/nsDocLoader.cpp:643
> #15 0x000000010027a5a2 in nsLoadGroup::RemoveRequest (this=0x10f12a020, request=0x119b7a260, ctxt=0x0, aStatus=NS_OK) at /Users/adw/fx/netwerk/base/src/nsLoadGroup.cpp:685
> #16 0x0000000100ddc781 in nsDocument::DoUnblockOnload (this=0x119b8c000) at /Users/adw/fx/content/base/src/nsDocument.cpp:7939
> #17 0x0000000100ddc533 in nsDocument::UnblockOnload (this=0x119b8c000, aFireSync=true) at /Users/adw/fx/content/base/src/nsDocument.cpp:7867
> #18 0x0000000100ff192c in nsLoadBlockingAsyncDOMEvent::~nsLoadBlockingAsyncDOMEvent (this=0x11abff600) at /Users/adw/fx/content/events/src/nsAsyncDOMEvent.cpp:78
> #19 0x0000000100ff18b5 in nsLoadBlockingAsyncDOMEvent::~nsLoadBlockingAsyncDOMEvent (this=0x11abff600) at /Users/adw/fx/content/events/src/nsAsyncDOMEvent.cpp:76
> #20 0x0000000100ff1889 in nsLoadBlockingAsyncDOMEvent::~nsLoadBlockingAsyncDOMEvent (this=0x11abff600) at /Users/adw/fx/content/events/src/nsAsyncDOMEvent.cpp:76
> #21 0x000000010302a635 in nsRunnable::Release (this=0x11abff600) at nsThreadUtils.cpp:31
> #22 0x000000010001bffb in nsCOMPtr<nsIRunnable>::~nsCOMPtr (this=0x7fff5fbfc648) at nsCOMPtr.h:514
> #23 0x000000010001a495 in nsCOMPtr<nsIRunnable>::~nsCOMPtr (this=0x7fff5fbfc648) at nsCOMPtr.h:511
> #24 0x00000001030d3f23 in nsThread::ProcessNextEvent (this=0x10dfa60e0, mayWait=false, result=0x7fff5fbfc6f3) at /Users/adw/fx/xpcom/threads/nsThread.cpp:628
> ...

So in the first stack, CreateAboutBlankContentViewer ends up stopping the uri2 load.  In the second, the uri2 load stops normally.  (I think.  I'm no doc-loading expert.  At least differently, and by all appearances normally.)

Comment 10

[Drew Willcoxon :adw]

Mark rightly points out that step 2 in the sequence above -- replaced with a synchronous call to docShell.createAboutBlankContentViewer(null), instead of an async load(about:blank) -- can't possibly cause step 4 to happen later.  But CreateAboutBlankContentViewer is definitely there in the failing stack on step 4.  I've repeated it a bunch of times now.

The part of the failing stack I omitted above the CreateAboutBlankContentViewer frame happens to look just like the non-failing stack.  And the JS stack shows (automatically dumped when the `debugger` keyword is hit, and which I verified by calling DumpJSStack()):

> 0 anonymous(webProgress = [xpconnect wrapped (nsISupports, nsIDocShell, nsIWebNavigation, nsIDocumentLoader, nsIInterfaceRequestor, nsIWebProgress) @ 0x11753e160 (native @ 0x111550408)], req = [xpconnect wrapped nsIRequest @ 0x11aca4740 (native @ 0x11a64a830)], flags = 786448, status = 2152398850, [...] <Failed to get 'length' while inspecting stack frame>
> ) ["chrome://global/content/backgroundPageThumbsContent.js":84]
> 1 anonymous(    <Failed to get 'arguments' while inspecting stack frame>
> ) ["chrome://global/content/backgroundPageThumbsContent.js":132]

Frame 0 (line 84) is the debugger keyword that I added to the top of onStateChange.  frame 1 (line 132) is the docShell.createAboutBlankContentViewer(null) call.

So: when uri2 STOP happens, both the native stack and the JS stack show createAboutBlankContentViewer on the stack.  Something weird is happening, like reentrancy or something.  Or I have some giant misunderstanding in debugging this.

Comment 11

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #10)
> So: when uri2 STOP happens, both the native stack and the JS stack show
> createAboutBlankContentViewer on the stack.

And to be clear, the native stack shows not only the createAboutBlankContentViewer, but above that, some page that has finished loading normally.

Comment 12

[Drew Willcoxon :adw]

This places DB should be enough to reproduce: http://people.mozilla.org/~adw/914920-places.sqlite

I've been doing:

1. rm the cached thumbnail files from the profile
2. start Firefox
3. open a new tab

There should be four top sites.  The first two are Bank of America and Wikipedia.  When the problem happens, Bank of America pops in, and then Wikipedia quickly follows, except it's the thumbnail for Bank of America.

I can reproduce maybe once out of 5-10 tries.  When the Wikipedia thumbnail loads as expected, I quit Firefox and do the STR again.

Comment 13

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Attachment: Wait for about:blank to load before sending capture back

I'm not 100% sure exactly what is going on here.  At a minimum, I saw the BOA page do a window.location.reload() on a timer.  It wouldn't surprise me to find this continued to send events we weren't seeing.  Another possibility is that the order of onStateChange events is somewhat non-deterministic as some are generated directly by the child process, while others are generated based on messages from the parent.

Regardless, I think a solution would be to only send the capture data *after* about:blank completes loading - at this point we would expect that no further events from the content would come (although if the events really are out-of-order, then that might not be true).

Anyway, the attached patch means I can no longer reproduce the problem.  I haven't really self-reviewed it yet - there are some obvious cleanups that could be done, and the tests would need to be run.  Drew, let me know what you think - if you think it's OK, i'll either clean it up and request review or you could take it, tweak it, then I can review it :)

Comment 14

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Mark Hammond [:markh] from comment #13)

typo:

> It wouldn't surprise me
> to find this continued to send events we weren't seeing.  Another

I meant "we weren't expecting"

Comment 15

[Drew Willcoxon :adw]

Attachment: Wait for about:blank to load before sending capture back v2

Thanks, Mark.  Loading about:blank and waiting for it to stop at the end of each capture is an idea I also had, so I think this is good even if we don't exactly know what's going on.  This patch basically refactors yours a little.  The other change is deleting this._captureInfo when the fileReader starts, not when it finishes, which I think should be safer by preventing captures from finishing multiple times if for some reason onStateChange for about:blank is called while the fileReader is working.

I can't reproduce the bug with this either, and tests pass locally.

Comment 16

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8334287 [details] [diff] [review]
Wait for about:blank to load before sending capture back v2

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

::: toolkit/components/thumbnails/content/backgroundPageThumbsContent.js
@@ +61,5 @@
>                           Ci.nsIWebNavigation.LOAD_FLAGS_STOP_CONTENT,
>                           null, null, null);
>      // If a page was already loading, onStateChange is synchronously called at
>      // this point by loadURI.
> +

one thing I noticed while debugging is that the first few onStateChange calls happen synchronously while calling .loadURI - what do you think about moving the init of this._request to above the loadURI call?  IIUC, it should be impossible for this to cause any actual problems as we should never be in onCapture while a request is pending.

I'm not fussed about this though.

Comment 17

[Drew Willcoxon :adw]

There's one case if I'm not mistaken: timeouts due to long page loads.  We receive a capture request, start loading the page, 30s later the load hasn't finished and we get another request.  At that time, this._request is non-null.  If loadURI causes onStateChange to be synchronously called, this._request can't be the old request, because it should be ignored, and it can't be the new request, because its page hasn't loaded yet.  So that's why I delete this._request before loadURI and set the new this._request after.

Comment 18

[Drew Willcoxon :adw]

https://tbpl.mozilla.org/?tree=Try&rev=7d3d39ba8478

Comment 19

[Drew Willcoxon :adw]

Attachment: Wait for about:blank to load before sending capture back v3

This keeps a queue of captures.  It loads about:blank even after canceled captures and always waits for about:blank to finish loading before starting the next capture.  This should fix the problem you pointed out in the previous patch of not waiting for about:blank when captures are canceled.

Comment 20

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8338238 [details] [diff] [review]
Wait for about:blank to load before sending capture back v3

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

Why do we need a "real" queue here?  ISTM it will only ever have 1 element in it - that when a capture request comes in, we need to remember it and wait for about:blank to load before processing it, but we should never get another capture request until we've done with that one.  That would seem to simplify the logic somewhat.

Comment 21

[Drew Willcoxon :adw]

It's much easier for me to reason about a queue than to convince myself that we don't need one in any case ever.  And I'd like for the content script to be self-contained and internally consistent and not make many assumptions about how the parent implementation is going to use it.  And I don't think that keeping a _currentCapture is actually much simpler in practice than keeping a _captureQueue.

Comment 22

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8338238 [details] [diff] [review]
Wait for about:blank to load before sending capture back v3

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

I have to disagree about this.  The current code already has an assumption that the next request will not come in until the previous has been notified.  Adding a queue is just additional complexity that we simply don't need - it's never actually used as a queue, but casual readers of the code will assume it is - and then wonder what we were thinking by incorporating a queue on *both* the parent side and the child side (ie, if you were *moving* the queue from the parent to the child I'd be more sympathetic).  In this patch, each queue element has its own state (which would be pointless even if there *was* a real queue - you couldn't ever have 2 queue entries with CAPTURE_STATE_CAPTURING) - there's alot of unnecessary complexity here.

So I have to disagree that *pretending* there is a queue is either simpler in practice or internally consistent.

Comment 23

[Drew Willcoxon :adw]

The fact is that a queue follows from our decision to load about:blank when finishing captures.

When a new request is received but _currentRequest's page is still loading, we cancel the load by loading about:blank and waiting for about:blank to finish loading.  What do we do with the new request in the meantime?  Stick it in a _nextRequest?  At that point you've got queue.  It's got two items in it.  You can pretend it's not a queue, or artificially limit it to a maximum of two items, but why?

Capture state is orthogonal to whether or not the code calls out the fact that there's a queue.

Comment 24

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Attachment: t2.patch

(In reply to Drew Willcoxon :adw from comment #23)

> When a new request is received but _currentRequest's page is still loading,
> we cancel the load by loading about:blank and waiting for about:blank to
> finish loading.

Right - but at that point we throw the existing "current" request away.

> What do we do with the new request in the meantime?  Stick
> it in a _nextRequest?  At that point you've got queue.  It's got two items
> in it.

I don't see that - when a new request comes in, we throw everything we know about the "current" request away and the new request becomes current.  At that point, the capture *window* is in a state where we are waiting for about:blank to finish loading (so we haven't actually started the new "current" request yet)

> Capture state is orthogonal to whether or not the code calls out the fact
> that there's a queue.

I'm not quite sure what you mean here, but my point was that the "state" applies to the window/docshell etc, so even if there was a queue, it doesn't seem to make sense that each queue item has a state.

Anyway, code speaks louder than words.  This patch is based on my earlier one, and should also handle this "request timed out" issue.  It has no queue, no explicit state variables (although this._currentCaptureRequest and this._captureInfo are proxies for this state) and seems (to me) to be significantly simpler.

Comment 25

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8338888 [details] [diff] [review]
t2.patch

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

::: toolkit/components/thumbnails/content/backgroundPageThumbsContent.js
@@ +56,5 @@
>      return docShell.QueryInterface(Ci.nsIWebNavigation);
>    },
>  
> +  _startCapture: function() {
> +    let msg = this._currentCaptureRequest;

oops - this line should be removed.

Comment 26

[Drew Willcoxon :adw]

Comment on attachment 8338888 [details] [diff] [review]
t2.patch

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

::: toolkit/components/thumbnails/content/backgroundPageThumbsContent.js
@@ +70,5 @@
>  
> +  _onCapture: function (msg) {
> +    let haveExisting = !!this._currentCaptureRequest;
> +    this._currentCaptureRequest = msg.json;
> +    if (haveExisting) {

You null out _currentCaptureRequest when toBlob finishes.  You load about:blank after that, so _onCapture can end up calling _startCapture while you're waiting for the previous capture's about:blank to load.  Similarly, _onCapture can load a canceling about:blank while the current capture is toBlob'ing.

This is why my patch has state per capture.

Comment 27

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Drew Willcoxon :adw from comment #26)
> You null out _currentCaptureRequest when toBlob finishes.  You load
> about:blank after that, so _onCapture can end up calling _startCapture while
> you're waiting for the previous capture's about:blank to load.  Similarly,
> _onCapture can load a canceling about:blank while the current capture is
> toBlob'ing.

Yes, that's true - which could be fixed with a variable indicating if we are waiting for about:blank to complete (which is the state of the window itself)

> This is why my patch has state per capture.

I don't see how that follows.  The state is the state of the window, which implies where we are at capturing the single item we are currently doing.  It's not as though different items in the queue can have truly distinct states.  Eg, in your patch, there might be 2 items in the queue for a brief period, which itself implies the head *must* have a state of CAPTURE_STATE_QUEUED and the second *must* have CAPTURE_STATE_CANCELED.  IOW, the state of the items in the queue simply reflect the state of the window, but with additional and unnecessary complexity.

Regardless, I still maintain that using a queue here is the wrong thing to do given how the parent and content processes interact (they form parts of the same system and have no relevance or function in isolation, and there's no future scenario I can see where they could be used in isolation).

Comment 28

[Drew Willcoxon :adw]

Like I pointed out, there's a queue here whether you deny it or not.  It doesn't have anything to do with the queue in the parent.  And you can scatter state across a bunch of variables or you can keep a single state variable.  I know which one I think is more complex.

(In reply to Mark Hammond [:markh] from comment #27)
> I don't see how that follows.

If the code uses an explicit queue, and every item in the queue is of the same type, then it makes sense to attach state to the items in it, even if every item after the head has the same state.  I'm willing to budge on this point, though, and move the state to some global.

> states.  Eg, in your patch, there might be 2 items in the queue for a brief
> period, which itself implies the head *must* have a state of
> CAPTURE_STATE_QUEUED and the second *must* have CAPTURE_STATE_CANCELED.

This is tangential, but actually, all the items after the head will be QUEUED.  Once started, the head can be in any of the other states (and will remain in the queue until it finishes), no matter whether there are other captures queued behind it.

Comment 29

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Drew Willcoxon :adw from comment #28)
> Like I pointed out, there's a queue here whether you deny it or not.

This seems to be our sticking point.  The way I see it, there is exactly 1 "current" request, but we may not be able to start processing that request until the state of the window is that about:blank has completed loading.  That's not what I'd call a queue.

I'm not sure how to proceed here, other than to r+ a patch which I believe adds unnecessary complexity, and I'm not willing to do that.  I understand that must be frustrating for you - maybe another reviewer would be best?

Comment 30

[Drew Willcoxon :adw]

Attachment: Wait for about:blank to load before sending capture back v4

Comment 31

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8342754 [details] [diff] [review]
Wait for about:blank to load before sending capture back v4

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

Thanks - I think this is much easier to grok.

::: toolkit/components/thumbnails/content/backgroundPageThumbsContent.js
@@ +98,5 @@
> +          // about:blank has loaded, ending the current capture.
> +          this._finishCurrentCapture();
> +          delete this._currentCapture;
> +          this._startNextCapture();
> +        }

nit: the elses should be on the same line as the closing brace - https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style

Comment 32

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/fx-team/rev/4727303dd8a0

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4727303dd8a0

Comment 34

[Drew Willcoxon :adw]

Comment on attachment 8342754 [details] [diff] [review]
Wait for about:blank to load before sending capture back v4

This patch applies cleanly to Aurora.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 901294
User impact if declined: Users will sometimes see the wrong thumbnails for sites on about:newtab.  Since this patch landed on 28 and background thumbnails were enabled on beta/release on 27, this bug will be present for one desktop Firefox release (27) if this patch isn't uplifted.
Testing completed (on m-c, etc.): automated testing on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 35

[bhavana bajaj [:bajaj]]

(In reply to Drew Willcoxon :adw from comment #34)
> Comment on attachment 8342754 [details] [diff] [review]
> Wait for about:blank to load before sending capture back v4
> 
> This patch applies cleanly to Aurora.
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): bug 901294
> User impact if declined: Users will sometimes see the wrong thumbnails for
> sites on about:newtab.  Since this patch landed on 28 and background
> thumbnails were enabled on beta/release on 27, this bug will be present for
> one desktop Firefox release (27) if this patch isn't uplifted.
> Testing completed (on m-c, etc.): automated testing on m-c
> Risk to taking this patch (and alternatives if risky): low
> String or IDL/UUID changes made by this patch: none

Is https://bugzilla.mozilla.org/show_bug.cgi?id=901294 the correct regressing bug here ? Looks like firefox 26 will be impacted in that case.Correct ?

Comment 36

[Drew Willcoxon :adw]

Bug 901294 is the bug that introduced the code that caused the problem, but the background thumbnailing feature was disabled on beta and release until 27.  i.e., beta and release users will get background thumbnailing starting in 27.

Comment 37

[bhavana bajaj [:bajaj]]

(In reply to Drew Willcoxon :adw from comment #36)
> Bug 901294 is the bug that introduced the code that caused the problem, but
> the background thumbnailing feature was disabled on beta and release until
> 27.  i.e., beta and release users will get background thumbnailing starting
> in 27.

Thanks for clarifying ! Makes sense now.

Comment 38

[Drew Willcoxon :adw]

Thanks!

https://hg.mozilla.org/releases/mozilla-aurora/rev/24fb5c627c02

Comment 39

[:Gavin Sharp [email: [e-mail]]]

(26 was unaffected because bg thumbnailing is disabled there)

Comment 40

[u279076]

Chris, are you able to reproduce this anymore with Firefox 27 and 28?

Comment 41

[Chris Peterson [:cpeterson]]

I haven't seen this thumbnail problem in a long time, so WFM. :)

Comment 42

[u279076]

Thanks Chris.