[tbb-bugs] #13439 [Tor Browser]: Inspector raises the canvas prompt when hovering over images

[Tor Bug Tracker & Wiki]

#13439: Inspector raises the canvas prompt when hovering over images
-------------------------+-------------------------------------------------
     Reporter:  dcf      |      Owner:  tbb-team
         Type:  defect   |     Status:  needs_review
     Priority:  minor    |  Milestone:
    Component:  Tor      |    Version:
  Browser                |   Keywords:  tbb-easy, tbb-usability,
   Resolution:           |  TorBrowserTeam201412R
Actual Points:           |  Parent ID:
       Points:           |
-------------------------+-------------------------------------------------

Comment (by gk):

 Replying to [comment:16 gacar]:
 > Replying to [comment:15 gk]:
 > > gacar: thanks. One question: Why do you have
 > > {{{
 > > JS::DescribeScriptedCaller(aCx, &scriptFile, &scriptLine)
 > > }}}
 > > wrapped in an if-clause? Reading the code this means to me this call
 can fail but *not* for
 > > third-party content as you are assuming `scriptFile.get()` and
 `scriptLine` are available unconditionally in this scenario. If that is
 correct which failing scenarios did you have in mind?
 > I didn't think about any specific scenario but just tried to be
 cautious.

 Ok, good. :)

 > When `DescribeScriptedCaller` returns false, we're sure that neither
 `scriptFile` nor `scriptLine` is updated with the caller script's URL and
 line no: http://mxr.mozilla.org/mozilla-esr31/source/js/src/jsapi.cpp#6259
 >
 > So there was no reason to continue with the `strcmp`, when `scriptFile`
 doesn't hold the real script URL.
 >
 > Does that make sense?

 Yes, but I was not talking about that part. I was wondering about:
 {{{
         nsPrintfCString msg("On %s: blocked access to canvas image data"
                             " from document %s, script from %s:%u ",  //
 L10n
                             firstPartySpec.get(), docSpec.get(),
                             scriptFile.get(), scriptLine);
 }}}
 where the message is constructed with the script file and the respective
 line *regardless* whether `DescribeScriptedCaller` succeeded or not. It
 seems to me if you wrap the former in an if-clause to be defensive (which
 is good, especially as Mozilla has not our patch in its repository yet and
 might change behavior of code we need without us noticing it) you should
 make sure `scriptFile` is updated when constructing the message as well
 (if not we might just give back a generic message without it).

 One additional nit: Mozilla code breaks usually at 80 chars. I think we
 should follow that rule. It might make it a bit easier to upstream things
 without spending another review cycle fixing the line-wrapping.

 Now: does that make sense?

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/13439#comment:17>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online